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(plugin-legacy): prevent global process.env.NODE_ENV mutation #9741

Merged
merged 4 commits into from Aug 24, 2022

Conversation

wucdbm
Copy link
Contributor

@wucdbm wucdbm commented Aug 18, 2022

Description

TL;DR Enabling the legacy plugin will always cause process.env.NODE_ENV to mutate to production and this is a problem when running several builds in series programmatically in 3rd-party CLI apps.

  • Call Vite's build function
  • build calls resolveConfig and passes defaultMode = production
  • The resolveConfig function overwrites process.env.NODE_ENV and sets it === production if mode === production
  • The legacy plugin does not pass mode into vite build's config, so it just defaults to production and then resolveConfig overwrites my NODE_ENV env variable that I had previously set on my CLI
  • This is bad not only because it modifies global vars as a general bad practice (I realize why this is needed!), but also because it prevents me from running NODE_ENV=development my-cli-app build where my-cli-app will run Vite's build function several times as per my needs

Additional context

So I have a CLI app that runs Vite's build function several times one after another, generally most of the time it runs a build for a client set of assets, and another time for the SSR/server version. But when the legacy plugin is activated, it runs after the client build and overrides my NODE_ENV=development, which results in the SSR/server build always building in PROD mode.

git checkout main
pnpm build
git checkout fix-legacy-plugin-node-env-mutation
# will fail with main branch build
pnpm test-build playground/legacy/__tests__/client-and-ssr/
yarn build
# will succeed after fix
pnpm test-build playground/legacy/__tests__/client-and-ssr/

PS So I messed up the first commit's message, is it possible to squash and set another commit message that better describes the issue?!

PPS Updated PR to use the resolved vite config mode field instead of process.env.NODE_ENV directly.


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.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! This make sense to me and is definitely an issue in the bigger picture. We may be changing this in the future from this discussion but for now this fix looks fine to me.

The caveat now is that the polyfill chunk may be built in development mode, but I don't think it's a big issue if the whole bundle is built in development mode in the first place.

packages/plugin-legacy/src/index.ts Outdated Show resolved Hide resolved
@bluwy bluwy added plugin: legacy p3-minor-bug An edge case that only affects very specific usage (priority) labels Aug 21, 2022
@bluwy bluwy changed the title fix(plugin-legacy): Unintended global procerss.env.NODE_ENV mutation fix(plugin-legacy): prevent global process.env.NODE_ENV mutation Aug 24, 2022
@patak-dev patak-dev merged commit a8279af into vitejs:main Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority) plugin: legacy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants