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.

Sign up for GitHub

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

Jump to bottom

fix(config)!: support development build #11045

Merged
merged 7 commits into from Nov 29, 2022
Merged

fix(config)!: support development build #11045

merged 7 commits into from Nov 29, 2022

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Nov 23, 2022

Description

Followup to #10996. #10996 makes build run in production NODE_ENV by default regardless of mode. Since Vite only supports NODE_ENV=production in .env files, there's no way for a user to build in development NODE_ENV.

This PR removes support for NODE_ENV=production, but added support for NODE_ENV=development only. Allowing development builds.

Additional context

Context: Previously in Vite 3, if the user sets --mode staging, it builds in development NODE_ENV which wasn't a nice default so #10996 changed it.

I didn't add support for NODE_ENV=production as it doesn't make sense in most case. It's only useful for dev command, but production NODE_ENV typically don't work well with the dev command.

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 breaking change p3-significant High priority enhancement (priority) labels Nov 23, 2022
mtaron
mtaron reviewed Nov 23, 2022
View reviewed changes
docs/guide/env-and-mode.md Outdated Show resolved Hide resolved
@benmccann
Copy link
Collaborator

Why do we need a restriction on not using production NODE_ENV during development? Is it something in Vite core that would break or some plugin in the ecosystem? I think it'd be nice if we could remove this restriction, but at the very least, if we're going to keep it, I think we should log a warning that we're ignoring the user setting rather than doing it silently.

@bluwy
Copy link
Member Author

bluwy commented Nov 28, 2022

We're not supporting production NODE_ENV in dev because it breaks HMR with the Vue plugin (last I tested it reads NODE_ENV in the compiler). Adding a warning sounds like a good idea.

benmccann
benmccann reviewed Nov 28, 2022
View reviewed changes
Copy link
Collaborator

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

If it's just the Vue plugin that breaks, can we move the error there? I'm not sure we need to disallow it for everyone else

packages/vite/src/node/config.ts Show resolved Hide resolved
@bluwy
Copy link
Member Author

bluwy commented Nov 28, 2022

That might be a solution too. I guess we could still support NODE_ENV=production, but I'm not sure if there's a usecase for it especially in dev, we're still injecting dev related stuff that doesn't represent prod.

@benmccann
Copy link
Collaborator

I imagine people could have some code like if (import.meta.env.PROD) that would be nice to test without having to do a whole vite build each time they tweak it

patak-dev
patak-dev previously approved these changes Nov 28, 2022
View reviewed changes
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

I prefer that we allow NODE_ENV production during dev once we have a real use case. The warning making this a non-feature so people don't start relying on it seems like a safer choice at this point. @bluwy feel free to merge if you are ok with this 👍🏼

benmccann
benmccann previously approved these changes Nov 28, 2022
View reviewed changes
Copy link
Collaborator

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

Ok. Sounds fine to me. I did have one suggestion to add a comment above to explain it, but otherwise lgtm

@patak-dev
Copy link
Member

If we add a comment, I think we should add a more generic comment, like Blu's message:

We're not supporting production NODE_ENV in dev because it could break HMR in some frameworks (i.e. the Vue Plugin)

@bluwy bluwy dismissed stale reviews from benmccann and patak-dev via 5fe2f2d November 29, 2022 06:18
@patak-dev patak-dev merged commit 8b3d656 into main Nov 29, 2022
@patak-dev patak-dev deleted the support-dev-build branch November 29, 2022 06:32
rnathuji added a commit to openstax/k12-apps-raise that referenced this pull request Jan 13, 2023
@rnathuji
Update env usage for new conventions in Vite 4
3e29722 
There were a couple of improvements in the updated version of Vite
around decoupling build modes and NODE_ENV:

* vitejs/vite#10996
* vitejs/vite#11045

Accordingly, this change:

* Consistently uses import.meta.env.MODE to set build mode specific
  values vs relying on the (previous) behavior around how
  import.meta.env.PROD was set.
* Adds a development environment file so NODE_ENV is set to development
  for this builds.
futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023
@bluwy @futurGH
fix(config)!: support development build (vitejs#11045)
5b5508c
matej21 added a commit to contember/interface that referenced this pull request May 19, 2023
@matej21
fix vite dev build
9e92a2f 
import.meta.env.PROD (and DEV) are no longer determited by vite "mode"

vitejs/vite#11045
matej21 added a commit to contember/interface that referenced this pull request May 22, 2023
@matej21
fix vite dev build
06b182e 
import.meta.env.PROD (and DEV) are no longer determited by vite "mode"

vitejs/vite#11045
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtaron mtaron mtaron left review comments

@patak-dev patak-dev patak-dev approved these changes

@benmccann benmccann benmccann left review comments

Assignees
No one assigned
Labels
breaking change p3-significant High priority enhancement (priority)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bluwy @benmccann @patak-dev @mtaron