Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(defineOption): only allow string values #6052

Closed
wants to merge 3 commits into from
Closed

fix(defineOption): only allow string values #6052

wants to merge 3 commits into from

Conversation

ydcjeff
Copy link
Contributor

@ydcjeff ydcjeff commented Dec 10, 2021

Description

Only allow string values in define option as stated in docs. Why made this change is I understand define option's values should be string as per docs. But in the plugin-vue, it is written as boolean and in the define plugin, it converts string if it is not. For a contributor, this is making a subtle difference between the docs and the codebase.
Also, TS doesn't give type error for non string values of define option. So, I think it is good to unify the internal usage and the docs.

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@ygj6
Copy link
Member

ygj6 commented Dec 10, 2021

A breaking change, I thought, the define feat does hava inconsistent behavior, For reference (copy from #5570):

From vite's docs

Starting from 2.0.0-beta.70, string values will be used as raw expressions, so if defining a string constant, it needs to be explicitly quoted (e.g. with JSON.stringify).

From esbuild's docs

Replacement expressions must either be a JSON object (null, boolean, number, string, array, or object) or a single identifier.

IMO, only allow string values in define option seems not very desirable.

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Dec 10, 2021

Yes, this became a breaking change for those who rely on the internal string conversion.

Can you point me to the code where esbuild define is used? I tried finding but can't find it. Thanks.

IMO, only allow string values in define option seems not very desirable.

Since there is the internal string conversion, so it might be better to only allow string values to match the docs behavior and avoid the issues like #5150.

@ygj6
Copy link
Member

ygj6 commented Dec 10, 2021

Can you point me to the code where esbuild define is used?

FYI, although only allowing string values will not affect esbuild

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Dec 10, 2021

Thanks!

I just noticed define passed to esbuild is converted to string. So, I think it is ok to allow only string (at least for now).

@jonaskuske
Copy link
Contributor

Maybe updating the docs and preventing a breaking change might be a better idea here, mentioning that non-string values are converted to strings using JSON.stringify()?

Either way, I'd recommend to submit the fix for @vitejs/plugin-vue in a separate PR as it doesn't have anything to do with whether define only takes strings or not

@bluwy
Copy link
Member

bluwy commented Dec 16, 2021

I second updating the documentation too to prevent a breaking change. And to fix the types for define in the docs.

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Jan 3, 2022

Closing it as it could be a breaking change.

@ydcjeff ydcjeff closed this Jan 3, 2022
@ydcjeff ydcjeff deleted the fix/define-string-only branch January 3, 2022 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants