fix(defineOption): only allow string values

[ydcjeff]

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]

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]

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]

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

vite/packages/vite/src/node/optimizer/index.ts

Line 305 in e1d82fc

define,

FYI, although only allowing string values will not affect esbuild

[ydcjeff]

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]

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]

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

[ydcjeff]

Closing it as it could be a breaking change.