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(optimizer): hash by config mode only #8878

Closed
wants to merge 1 commit into from

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Jul 1, 2022

Description

Fix regression from #7673

When hashing the mode, use config.mode only. process.env.NODE_ENV is different than mode.

Otherwise this could cause unnecessary reloads on warm starts.

Additional context

I think past me made a brain-fart.


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.

@bluwy bluwy added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jul 1, 2022
@netlify
Copy link

netlify bot commented Jul 1, 2022

👷 Deploy Preview for vite-docs-main processing.

Name Link
🔨 Latest commit e47212b
🔍 Latest deploy log https://app.netlify.com/sites/vite-docs-main/deploys/62bea8e54aa218000a269447

@netlify
Copy link

netlify bot commented Jul 1, 2022

Deploy Preview for vite-docs-main canceled.

Name Link
🔨 Latest commit e47212b
🔍 Latest deploy log https://app.netlify.com/sites/vite-docs-main/deploys/62bea8e54aa218000a269447

1 similar comment
@netlify
Copy link

netlify bot commented Jul 1, 2022

Deploy Preview for vite-docs-main canceled.

Name Link
🔨 Latest commit e47212b
🔍 Latest deploy log https://app.netlify.com/sites/vite-docs-main/deploys/62bea8e54aa218000a269447

@patak-dev
Copy link
Member

We still need this in dev though, see:

  // esbuild automatically replaces process.env.NODE_ENV for platform 'browser'
  // In lib mode, we need to keep process.env.NODE_ENV untouched, so to at build
  // time we replace it by __vite_process_env_NODE_ENV. This placeholder will be
  // later replaced by the define plugin
  const define = {
    'process.env.NODE_ENV': isBuild
      ? '__vite_process_env_NODE_ENV'
      : JSON.stringify(process.env.NODE_ENV || config.mode)
  }
  
  // Build with esbuild using `define`

Maybe we should review why we are not using config.mode directly for deps optimization. Or another option is to always define to __vite_process_env_MODE_ENV (not only in build), and then define that as a global. Maybe that is the best so we can remove safely the mode.

@bluwy
Copy link
Member Author

bluwy commented Jul 3, 2022

We only need to do process.env.NODE_ENV || config.mode for NODE_ENV only to match how define plugin works. The change in this PR is to mode specifically, which should be config.mode only. It was a mistake in my PR ago 😅

Though I also think it's worth investigating why process.env.NODE_ENV || config.mode yields different results between cold and warm starts, but I think at the meantime this fix helps prevent an edge case for now.

Re using __vite_process_env_MODE_ENV for dev too, I think that could be worth trying out too since treeshaking isn't necessary for dev.

@patak-dev
Copy link
Member

If process.env.NODE_ENV is used in the optimized bundles, then we need to include it in the hash. If not, once it changes, it will not be reflected, or am I missing something?

process.env.NODE_ENV || config.mode yields different results between cold and warm starts

Really puzzled by this one. It should be easy to debug in your setup to check what is going on 🤔

Re using __vite_process_env_MODE_ENV for dev too, I think that could be worth trying out too since treeshaking isn't necessary for dev.

I think we should go with this approach so we remove these from the hash. If you prefer, I can send a PR with that.

@bluwy
Copy link
Member Author

bluwy commented Jul 3, 2022

When I test #8869, it doesn't seem to happen anymore. Though whenever I log process.env.NODE_ENV when Vite generates the dep hash and when running esbuild optimize, I get undefined for both strangely.

For Vite 2.9, I log in the same locations, and dep hash returns undefined too, but esbuild optimize gets the actual value development.

Reference:

dep hash:

mode: process.env.NODE_ENV || config.mode,

esbuild optimize:

'process.env.NODE_ENV': JSON.stringify(process.env.NODE_ENV || config.mode)

This is really strange, and perhaps we're calling the optimizer too early. But anyways this shouldn't be an issue with #8869 merged so closing this.

@bluwy bluwy closed this Jul 3, 2022
@bluwy bluwy deleted the fix-hash-mode branch July 3, 2022 07:32
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants