-
-
Notifications
You must be signed in to change notification settings - Fork 621
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat!: rename --define-process-env-node-env
to --config-node-env
#4318
Conversation
759901b
to
5544aaf
Compare
--node-env
and `--define-process-env-n…--node-env
and --define-process-env-node-env
--node-env
and --define-process-env-node-env
--node-env
and --define-process-env-node-env
} else if (typeof options.nodeEnv === "string") { | ||
const isNodeEnvOptionDefined = typeof options.nodeEnv === "string"; | ||
const isDefineProcessEnvNodeEnvOptionDefined = | ||
typeof options.defineProcessEnvNodeEnv === "string"; |
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.
@snitin315 I don't think we need this option anymore anyway. We have - https://github.com/webpack/webpack/blob/main/lib/WebpackOptionsApply.js#L599.
The main problem is that we have two options:
--node-env
(setprocess.env.NODE_ENV
forwebpack.config.js
)--optimization-node-env
(setprocess.env.NODE_ENV
for runtime)
And that's why they confuse some developers. The names are almost the same, but they do completely different things.
I think the best solution would be to do the following:
- remove
--define-process-env-node-env
at all - we leave
--node-env
because it is widely used by many - we come up with a new name for the
--node-env
variable and updates the documentation so that new developers can use it
Shortly --node-env
and ---new-name-node-env
work and do the same thing, but the documentation will recommend to use a new name.
To be honest I have no ideas for a new name, so I'd be happy to hear any suggestions 😄
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.
@alexander-akait how about --config-node-env
or --config-only-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.
--config-node-env
sounds good
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.
Updated 👍🏻
d34d6e3
to
1ee224c
Compare
--node-env
and --define-process-env-node-env
--define-process-env-node-env
to --config-node-env
Hello, I've just realized one of our production scripts was broken because of this change, after upgrading to 6.x a couple of days ago. What do you think about adding this as a BREAKING CHANGE in the CHANGELOG of 6.0.0 ? It will make it easier for everyone to migrate while upgrading. Note that the previous change regarding this flag was mentioned in the CHANGELOG of 5.0.0. |
I second this. The --define-process-env-node-env option is still mentioned in the README.md, which could lead to confusion. |
What kind of change does this PR introduce?
Fix #3503 (comment)
webpack-cli/packages/webpack-cli/src/webpack-cli.ts
Lines 2388 to 2390 in c97779b
Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
Will update in docs website, I have updated the option descriptions.
Summary
#4318 (comment)
Does this PR introduce a breaking change?
Yes
--define-process-env-node-env
was removed in favor of--config-node-env
option.Other information
No