-
-
Notifications
You must be signed in to change notification settings - Fork 6k
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: support set NODE_ENV in scripts when custom mode option #8218
Conversation
6299677
to
81acd9d
Compare
packages/vite/src/node/config.ts
Outdated
@@ -395,7 +395,9 @@ export async function resolveConfig( | |||
// Note it is possible for user to have a custom mode, e.g. `staging` where | |||
// production-like behavior is expected. This is indicated by NODE_ENV=production | |||
// loaded from `.staging.env` and set by us as VITE_USER_NODE_ENV | |||
const isProduction = (process.env.VITE_USER_NODE_ENV || mode) === 'production' | |||
const isProduction = | |||
(process.env.VITE_USER_NODE_ENV || process.env.NODE_ENV || mode) === |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should process.env.NODE_ENV
be the first? Otherwise if NODE_ENV
is define in both CLI args and .env
, the .env
would take priority. I'm not sure if this causes any side effects, but I also don't remember why we have VITE_USER_NODE_ENV
in the first place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started by putting process.env.NODE_ENV in the first place, will causes ci fail https://github.com/vitejs/vite/runs/6491327150?check_suite_focus=true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why, I guess this test-serve
would set the default NODE_ENV.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vitest sets NODE_ENV
to mode || 'test'
when NODE_ENV
is not defined.
https://github.com/vitest-dev/vitest/blob/4701e0b9b07728fc64189b655c53708346ac9293/packages/vitest/src/node/cli-api.ts#L18
Maybe this is affecting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't NODE_ENV
be just set here
vite/packages/vite/src/node/config.ts
Lines 1011 to 1017 in 79a8c85
} else if ( | |
key === 'NODE_ENV' && | |
process.env.VITE_USER_NODE_ENV === undefined | |
) { | |
// NODE_ENV override in .env file | |
process.env.VITE_USER_NODE_ENV = value | |
} |
to
process.env.VITE_USER_NODE_ENV ?? process.env.NODE_ENV ?? value
instead in order to both keep respecting externally set VITE_USER_NODE_ENV
and allow externally set NODE_ENV
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The effect is actually the same
If I'm not mistaken, there's a slight difference.
Currently, when mode
is used, and both NODE_ENV
and VITE_USER_NODE_ENV
are set as system env vars (not in .env file), VITE_USER_NODE_ENV
takes precedence.
The change proposed in this PR seems to flip the precedence
- const isProduction = (process.env.VITE_USER_NODE_ENV || mode) === 'production'
+ const isProduction =
+ (process.env.NODE_ENV || process.env.VITE_USER_NODE_ENV || mode) ===
+ 'production'
It potentially may affect someone using VITE_USER_NODE_ENV
currently.
The change may be acceptable given that the case is pretty obscure, but I feel like it should be taken into consideration.
And once externally set NODE_ENV
is respected, there doesn't seem to be a reason for VITE_USER_NODE_ENV
to exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The effect is actually the same
If I'm not mistaken, there's a slight difference.
Currently, when
mode
is used, and bothNODE_ENV
andVITE_USER_NODE_ENV
are set as system env vars (not in .env file),VITE_USER_NODE_ENV
takes precedence.The change proposed in this PR seems to flip the precedence
- const isProduction = (process.env.VITE_USER_NODE_ENV || mode) === 'production' + const isProduction = + (process.env.NODE_ENV || process.env.VITE_USER_NODE_ENV || mode) === + 'production'It potentially might affect someone using
VITE_USER_NODE_ENV
currently.The change may be acceptable given that the case is pretty obscure, but I feel like it should be taken into consideration.
And once externally set
NODE_ENV
is respected, there doesn't seem to be a reason forVITE_USER_NODE_ENV
to exist.
I think VITE_USER_NODE_ENV
is a property provided for internal use, there is no description of VITE_USER_NODE_ENV
in the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't
build
always set it toproduction
unless overruled by externalNODE_ENV
? IfNODE_ENV
isn't set, and it isbuild
just set it toproduction
otherwise usemode | <VITEST>
NODE_ENV = NODE_ENV ?? (isBuild ? 'production' : mode )
Or am I missing something?
Sorry I forgot to reply.
This docs implies that the default NODE_ENV
is not production
even if it is build
. If the default NODE_ENV
were production
, NODE_ENV
does not need to be set with .env
.
https://vitejs.dev/guide/env-and-mode.html#modes
But we might need to consider which is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And @Dunqing, maybe good to justify why you choose the current approach instead of the one proposed by @swandir if the result is the same?
My intention is to make as few changes as possible to support this implementation, and if possible I can create a new pr separately to optimize this logic
Sounds good, I think we can merge this one then until we do a more holistic refactoring 👍🏼
Thanks for taking the time to address the questions 🙏🏼
81acd9d
to
da26930
Compare
e36dec2
to
647999e
Compare
647999e
to
7c39314
Compare
8c89c6a
to
120c091
Compare
Description
Support set NODE_ENV in scripts when custom mode option, e.g:
NODE_ENV=production vite build --mode custom && vite preview
Additional context
close: #8192
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).