feat(define): handle replacement with esbuild #11151

bluwy · 2022-12-02T08:27:02Z

Description

Use esbuild to handle define replacements

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

supersedes and closes #9791

define replacements are now replaced with esbuild. This matches the semantics of define vs replace, where define replace identifiers with a certain value (AST-informed), and replace does a simple find-and-replace (not AST-informed).

This may break if you specify define keys that are not single identifiers, and values that doesn't match esbuild's requirements. This former restriction is a breaking change, but the latter restriction is already documented today. We also (used to) pass define to esbuild for the optimizer too.

To be consistent with esbuild behavior, expressions must either be a JSON object (null, boolean, number, string, array, or object) or a single identifier.

Configurations that didn't follow this rule before used to indirectly work (especially in SSR), and they will not work with this PR. If they still need the previous behaviour, they can use https://www.npmjs.com/package/@rollup/plugin-replace that does replace instead.

If you've hit with parsing errors because you wrote import.meta.env.* in markdown before, this will now fix it.

Additional context

Tests are mostly copied from #9791 (Thanks Tony 😬)

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.

Co-authored-by: Tony Trinh <tony19@gmail.com>

bluwy · 2022-12-02T08:28:17Z

packages/vite/src/node/__tests__/plugins/define.spec.ts

    expect(await transform('const version = __APP_VERSION__;')).toBe(
-      'const version = "1.0";'
+      'const version = "1.0";\n'


Esbuild transform tend to semi-format the entire code so this is changed here. That also means we can't do stable sourcemaps transform.

bluwy · 2022-12-02T08:29:41Z

packages/vite/src/node/plugins/define.ts

-      'import.meta.env.': `({}).`,
      'import.meta.env': JSON.stringify(config.env),


This change is fine as esbuild will share a single reference to import.meta.env object as a variable, so it shouldn't increase the bundle size by much after bundling

Update: not exactly fine, as in the bundle size will technically increase, but multiple usages of import.meta.env should be deduplicated now. There isn't a way to fix the former with esbuild define restriction

bluwy · 2022-12-02T08:30:23Z

playground/define/__tests__/define.spec.ts

-  expect(await page.textContent('.import-meta-env-undefined')).toBe(
-    isBuild ? '({}).UNDEFINED' : 'import.meta.env.UNDEFINED'
-  )
-  expect(await page.textContent('.process-env-undefined')).toBe(
-    isBuild ? '({}).UNDEFINED' : 'process.env.UNDEFINED'
-  )


This test isn't relevant for esbuild transform anymore.

bluwy · 2022-12-02T09:00:30Z

So close 😢 Turns out esbuild strips out comments by default and there's no way to prevent that. That means /* @vite-ignore */ get strips out which makes the worker tests fail.

This currently also happens when the user uses a TypeScript file instead as esbuild transpiles it. Interestingly comments within import(/* @vite-ignore */) are not stripped.

Blocked by evanw/esbuild#221

bluwy · 2023-01-06T07:18:43Z

I realize this is a breaking change as define config not supported by esbuild, e.g. "foo.": "foo.bar.", will not work with this PR. That means we might have to implement another round of replacements, or break this intentionally. I'm slightly leaning towards the former though.

Setting this aside for now as the complexity is growing.

patak-dev · 2023-03-21T10:27:15Z

Adding this one to the Vite 5 milestone, if we move forward with it I think we should break compat. @bluwy maybe we could start issuing a warning for these cases in 4.3 or 4.4 to give time for folks to update their configs?

xrado · 2023-09-13T12:11:53Z

is this going into v5 ?

bluwy · 2023-10-12T13:35:35Z

playground/html/vite.config.js

@@ -38,7 +38,6 @@ export default defineConfig({
    'import.meta.env.VITE_NUMBER': 5173,
    'import.meta.env.VITE_STRING': JSON.stringify('string'),
    'import.meta.env.VITE_OBJECT_STRING': '{ "foo": "bar" }',
-    'import.meta.env.VITE_TEMPLATE_LITERAL': '`template literal`',


Removed this handling as it seems that this isn't a valid specifier for esbuild. We also documented this in the docs:

To be consistent with esbuild behavior, expressions must either be a JSON object (null, boolean, number, string, array, or object) or a single identifier.

This was added in #13425

bluwy · 2023-10-23T06:23:48Z

/ecosystem-ci run

vite-ecosystem-ci · 2023-10-23T06:43:00Z

📝 Ran ecosystem CI on 9d1772d: Open

suite	result	latest scheduled
analogjs	✅ success	✅ success
astro	✅ success	✅ success
histoire	✅ success	✅ success
ladle	✅ success	✅ success
laravel	❌ failure	❌ failure
marko	❌ failure	❌ failure
nuxt	❌ failure	✅ success
nx	❌ failure	❌ failure
previewjs	✅ success	✅ success
qwik	✅ success	✅ success
rakkas	❌ failure	✅ success
sveltekit	❌ failure	❌ failure
unocss	✅ success	✅ success
vike	❌ failure	❌ failure
vite-plugin-pwa	❌ failure	❌ failure
vite-plugin-react	✅ success	✅ success
vite-plugin-react-pages	❌ failure	✅ success
vite-plugin-react-swc	✅ success	✅ success
vite-plugin-svelte	❌ failure	✅ success
vite-plugin-vue	❌ failure	✅ success
vite-setup-catalogue	✅ success	✅ success
vitepress	✅ success	✅ success
vitest	❌ failure	❌ failure

docs/guide/migration.md

packages/vite/src/node/plugins/define.ts

ArnaudBarre · 2023-10-23T20:47:42Z

packages/vite/src/node/plugins/define.ts

+  // may contain values with raw identifiers, making it non-JSON-serializable,
+  // we replace it with a temporary marker and then replace it back after to
+  // workaround it. This means that esbuild is unable to optimize the `import.meta.env`
+  // access, but that's a tradeoff for now.


What is an example of may contain values with raw identifiers?
What is the cost of esbuild is unable to optimize?

What is an example of may contain values with raw identifiers?

We have this in plugin-legacy:

vite/packages/plugin-legacy/src/index.ts

Lines 211 to 214 in 297b425

'import.meta.env.LEGACY':

env.command === 'serve' || config.build?.ssr

? false

: legacyEnvVarMarker,

Where legacyEnvVarMarker is an __VITE_IS_LEGACY__ identifier to be replaced after transform in renderChunk. That means when we create a define for import.meta.env, the value is { ..., LEGACY: __VITE_IS_LEGACY__ } which is not valid JSON for esbuild. So we need to work-around it here.

What is the cost of esbuild is unable to optimize?

Here's an esbuild example. As you can notice, replacing with a JSON-object vs an identifier, JSON-objects can be deduplicated as a single variable. And perhaps among future optimizations where esbuild could treeshake object key access (now it doesn't).

And you think this would be to breaking to limit Vite define to the esbuild one?

I'm not sure I understand. We would have to continue making our plugins work, and the issue is only specific to import.meta.env, so a workaround is needed for import.meta.env specifically. The rest user-facing define rules still need to follow esbuild's rule.

antfu · 2023-10-24T02:58:54Z

I wonder if we could have both resolve.define and resolve.replace, where resolve.replace would be using @rollup/plugin-replace the same as the previous behavior. I think text replace is still a common need for a bundler to have (in docs we could encourage the use of define)

bluwy · 2023-10-24T05:05:45Z

Yeah I've also thought about that, but I think we could hold it for later if there's a common need for it. So far I've not seen many frameworks needing to use replace, and for users define should be good enough.

packages/vite/src/node/plugins/define.ts

bluwy · 2023-10-24T14:32:09Z

Something is wrong with the macos test. It's consistently failing with a RollupError: Cannot call "addWatchFile" after the build has finished. error somehow. Probably best to hold a while before merging.

bluwy · 2023-10-25T06:09:23Z

Update: The CI fail is unrelated and fixed at #14751

patak-dev

I think it is a good idea to merge this one soon if we want it in Vite 5 to get more time to test it

bluwy and others added 2 commits December 2, 2022 16:19

feat(define): handle replacement with esbuild

7217439

Co-authored-by: Tony Trinh <tony19@gmail.com>

docs: update define

8d00673

bluwy added the p3-significant High priority enhancement (priority) label Dec 2, 2022

bluwy commented Dec 2, 2022

View reviewed changes

bluwy marked this pull request as draft December 2, 2022 09:00

bluwy mentioned this pull request Dec 7, 2022

Allow arbitrary comments within dynamic imports and constructor calls evanw/esbuild#2721

Closed

bluwy added 2 commits January 6, 2023 11:19

Merge branch 'main' into define-with-esbuild

fd92514

chore: fix test

3724ce5

bluwy added the breaking change label Jan 6, 2023

bluwy mentioned this pull request Jan 12, 2023

Collections - generating page for MDX page fails withastro/astro#5703

Closed

1 task

fi3ework mentioned this pull request Feb 22, 2023

docs: define no longer follows the esbuild rules #12156

Closed

9 tasks

patak-dev added this to the 5.0 milestone Mar 21, 2023

bluwy mentioned this pull request Apr 1, 2023

feat: add option to exclude global define and env replacement #5577

Closed

9 tasks

auvred mentioned this pull request Jun 24, 2023

process.env.NODE_ENV replace bug #13597

Closed

7 tasks

bluwy added 5 commits October 12, 2023 16:06

Merge branch 'main' into define-with-esbuild

d86bc55

feat: fix everything

dc1e95c

perf: early out if possible

f86b4cc

chore: fix test

648c71b

chore: reduce diff

0d2daff

bluwy marked this pull request as ready for review October 12, 2023 13:31

bluwy commented Oct 12, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

fix: handle undefined

2554a47

bluwy added 2 commits October 23, 2023 15:12

fix: getHash length

ddd513a

fix: replace import.meta.env.SSR correctly

7ea6899

patak-dev reviewed Oct 23, 2023

View reviewed changes

docs/guide/migration.md Outdated Show resolved Hide resolved

patak-dev reviewed Oct 23, 2023

View reviewed changes

packages/vite/src/node/plugins/define.ts Outdated Show resolved Hide resolved

chore: update feedback

ea8a0f8

ArnaudBarre reviewed Oct 23, 2023

View reviewed changes

danielroe mentioned this pull request Oct 24, 2023

fix(vite): use rollup replace plugin for typeof optimisations nuxt/nuxt#23903

Merged

8 tasks

sapphi-red reviewed Oct 24, 2023

View reviewed changes

packages/vite/src/node/plugins/define.ts Outdated Show resolved Hide resolved

fix: enable sourcemap in dev

5c507af

Merge branch 'main' into define-with-esbuild

56d6260

patak-dev approved these changes Oct 26, 2023

View reviewed changes

bluwy merged commit e4c801c into main Oct 26, 2023
8 of 9 checks passed

bluwy deleted the define-with-esbuild branch October 26, 2023 03:30

smeng9 mentioned this pull request Oct 27, 2023

[5.0 beta 13] Issue with define replacement using esbuild #14783

Closed

7 tasks

bluwy mentioned this pull request Oct 28, 2023

perf(define): create simple regex for checks #14788

Merged

9 tasks

This was referenced Oct 31, 2023

chore: revert "feat: show warning to discourage putting process/global to define option (#14447)" #14827

Merged

docs: update stale define docs #14831

Merged

bluwy mentioned this pull request Nov 1, 2023

import.meta.env parse error when present in string for production build #14835

Closed

7 tasks

sapphi-red mentioned this pull request Nov 7, 2023

Build error when using json objects in define #10848

Closed

7 tasks

sapphi-red mentioned this pull request Nov 14, 2023

fix: don't escape dash in define regex #9980

Closed

9 tasks

bluwy mentioned this pull request Dec 4, 2023

chore: remove unused test files #15239

Merged

4 tasks

simonguo mentioned this pull request Jan 10, 2024

Failed to build the project with vite.js + rsuite rsuite/rsuite#3535

Open

raphaelameaume mentioned this pull request May 14, 2024

Build failing raphaelameaume/fragment#123

Closed

eltigerchino mentioned this pull request Nov 12, 2024

git commit message with reserved word cannot build in GitLab CI sveltejs/kit#6102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(define): handle replacement with esbuild #11151

feat(define): handle replacement with esbuild #11151

bluwy commented Dec 2, 2022 •

edited

Loading

bluwy Dec 2, 2022

bluwy Dec 2, 2022

bluwy Oct 12, 2023

bluwy Dec 2, 2022

bluwy commented Dec 2, 2022

bluwy commented Jan 6, 2023

patak-dev commented Mar 21, 2023

xrado commented Sep 13, 2023

bluwy Oct 12, 2023

This comment was marked as outdated.

This comment was marked as outdated.

bluwy commented Oct 23, 2023

vite-ecosystem-ci bot commented Oct 23, 2023

ArnaudBarre Oct 23, 2023

bluwy Oct 24, 2023

ArnaudBarre Oct 24, 2023

bluwy Oct 25, 2023

antfu commented Oct 24, 2023

bluwy commented Oct 24, 2023

bluwy commented Oct 24, 2023

bluwy commented Oct 25, 2023

patak-dev left a comment

		'import.meta.env.': `({}).`,
		'import.meta.env': JSON.stringify(config.env),

	'import.meta.env.LEGACY':
	env.command === 'serve' \|\| config.build?.ssr
	? false
	: legacyEnvVarMarker,

feat(define): handle replacement with esbuild #11151

feat(define): handle replacement with esbuild #11151

Conversation

bluwy commented Dec 2, 2022 • edited Loading

Description

Additional context

What is the purpose of this pull request?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluwy commented Dec 2, 2022

bluwy commented Jan 6, 2023

patak-dev commented Mar 21, 2023

xrado commented Sep 13, 2023

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

bluwy commented Oct 23, 2023

vite-ecosystem-ci bot commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antfu commented Oct 24, 2023

bluwy commented Oct 24, 2023

bluwy commented Oct 24, 2023

bluwy commented Oct 25, 2023

patak-dev left a comment

Choose a reason for hiding this comment

bluwy commented Dec 2, 2022 •

edited

Loading