feat(define): don't modify string literals

[tony19]

Description

The define and client-inject plugins indiscriminately replace strings within string literals, which can create invalid JavaScript syntax (e.g., console.log("mode is process.env.NODE_ENV") is transformed into console.log("mode is "process.env.NODE_ENV""). This PR strips the literals from the code string that the plugins scan, thereby leaving the literals unchanged, which is the same solution used in several other Vite plugins.

fix #9790 fix #3304 fix #8663 fix #6295 fix #9829

Additional context

This fix is similar to #9621, which was closed due to a preference for a regex fix for performance.

I did experiment with tweaking the plugin's regex to exclude string literals, but it's not robust enough to cover multiline strings:

const PUBLIC_TEST = "import.meta.env"  // ✅ ignored

const PUBLIC_TEST = `
import.meta.env.PUBLIC_TEST  // ❌ still matches
`

Plus, based on the description in #8054, it's impossible to handle this properly with regex.

---

What is the purpose of this pull request?

• [x] Bug fix
• [x] New Feature
• [x] Documentation update
• [ ] Other

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

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

[bluwy]

This fix is similar to #9621. But this using strip-literal means we're only tokenizing with acorn instead of a full parse. My opinion with either PRs is that I think this may be the only way for correctness in builds. And it's not too bad as we only need to tokenize on builds.

[patak-dev]

The difference with other places where we are using stripLiteral, is that we only use it after checking that there is a possible match (like checking for import.meta.url here). Maybe we should do the first match and then only do a stripLiteral to disard it? In this way, people that don't use define, won't pay for tokenizing every file.

Another thought is if we should combine some of the plugins that depends on stripLiteral so we avoid doing several passes on different plugins.

Let's add this one to be discussed with the team when we get the chance.

[FezVrasta]

Could someone help me understand if this is the same bug I'm observing in https://github.com/storybookjs/builder-vite/issues/489?

[tony19]

@FezVrasta That's not the same issue.

[patak-dev]

We discussed this PR in today's meeting @tony19, and we think that we should redefine the define feature as you did here to avoid replacing in strings.

Regarding the implementation, we discussed the performance implications and Evan had the idea to explore using Define from esbuild (directly in the esbuild plugin). So we should first look if that is a possible path forward (thus avoiding the tokenizer and regex scheme).

[shigma]

@patak-dev Using define from esbuild would be a very good idea. Currently vite is not really consistent with esbuild behavior (although documentation says so). Vite generate definitions into a single file and use references as replacements, which means:

• object definitions will share the same references
• users cannot provide context-aware definitions

In my case, I use resolve(__filename, '../client') in my source code (I want it to run on both node and browser), and trying to define __filename as import.meta.url, but it doesn't work. Using define from esbuild would certainly help.

[JulianCataldo]

I don't know why, but with me, this bugs occurs only with debian/ubuntu, not macOS

https://github.com/withastro/astro/issues/5074

[tony19]

@patak-dev I tried using esbuild's define option, but only valid identifiers are allowed, so these defines would cause an esbuild error because the identifiers end with .:

https://github.com/vitejs/vite/blob/23c9259aa31424d96fdc87f61236fd3b84aed2c3/packages/vite/src/node/plugins/define.ts#L21-L23 https://github.com/vitejs/vite/blob/23c9259aa31424d96fdc87f61236fd3b84aed2c3/packages/vite/src/node/plugins/define.ts#L52

This is the error I'm seeing:

vite v3.2.0-beta.3 building for production...
✓ 16 modules transformed.
[vite:esbuild-transpile] Transform failed with 8 errors:
error: The define key "globalThis.process.env." contains invalid identifier ""
error: Invalid define value (must be an entity name or valid JSON syntax): ({}).
error: The define key "import.meta.env." contains invalid identifier ""
error: Invalid define value (must be an entity name or valid JSON syntax): ({}).
error: The define key "process.env." contains invalid identifier ""
...

The define key "globalThis.process.env." contains invalid identifier ""

Invalid define value (must be an entity name or valid JSON syntax): ({}).

The define key "import.meta.env." contains invalid identifier ""

Invalid define value (must be an entity name or valid JSON syntax): ({}).

The define key "process.env." contains invalid identifier ""

Invalid define value (must be an entity name or valid JSON syntax): ({}).

The define key "global.process.env." contains invalid identifier ""

Invalid define value (must be an entity name or valid JSON syntax): ({}).

error during build:
Error: Transform failed with 8 errors:
error: The define key "globalThis.process.env." contains invalid identifier ""
error: Invalid define value (must be an entity name or valid JSON syntax): ({}).
error: The define key "import.meta.env." contains invalid identifier ""
error: Invalid define value (must be an entity name or valid JSON syntax): ({}).
error: The define key "process.env." contains invalid identifier ""
...
    at failureErrorWithLog (/Users/tony/src/vite/node_modules/.pnpm/[email protected]/node_modules/esbuild/lib/main.js:1566:15)
    at /Users/tony/src/vite/node_modules/.pnpm/[email protected]/node_modules/esbuild/lib/main.js:805:29
    at responseCallbacks.<computed> (/Users/tony/src/vite/node_modules/.pnpm/[email protected]/node_modules/esbuild/lib/main.js:671:9)
    at handleIncomingPacket (/Users/tony/src/vite/node_modules/.pnpm/[email protected]/node_modules/esbuild/lib/main.js:726:9)
    at Socket.readFromStdout (/Users/tony/src/vite/node_modules/.pnpm/[email protected]/node_modules/esbuild/lib/main.js:647:7)
    at Socket.emit (node:events:513:28)
    at addChunk (node:internal/streams/readable:315:12)
    at readableAddChunk (node:internal/streams/readable:289:9)
    at Socket.Readable.push (node:internal/streams/readable:228:10)
    at Pipe.onStreamRead (node:internal/stream_base_commons:190:23)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I could workaround the error by removing those "invalid identifiers" from esbuild's config, and then just use our own string replacement mechanism (with strip-literal) to handle those. This hybrid solution would still require scanning the code (more than once), so it's still suboptimal.

[patak-dev]

@patak-dev I tried using esbuild's define option, but only valid identifiers are allowed, so these defines would cause an esbuild error because the identifiers end with .:

I think we were using process.env. to avoid false positives with the current regex matching. We could go with the same rules as esbuild in Vite 4. For 4.0, we could still automatically remove the . and issue a warning that this is no longer valid and the extra dot needs to be removed, then in ~4.2 we do a hard switch.