-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
[WIP] Fixes #5392. EnvironmentPlugin now extends process.env #8721
[WIP] Fixes #5392. EnvironmentPlugin now extends process.env #8721
Conversation
For maintainers only:
|
I'm going to take a look at the failed tests |
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 think maybe that is because we are supposed to inject an env to a web browser, not node API wise
The basic integration tests failed. @misterdev Please review the following output log for errors:
See complete report here. |
I'm debugging this, I've seen the problem comes from the fact that the test has multiple configurations: module.exports = [
{
name: "aaa",
module: {
unknownContextRegExp: /$^/,
unknownContextCritical: false
},
plugins: [new EnvironmentPlugin("AAA")]
},
{
name: "bbbccc",
module: {
unknownContextRegExp: /$^/,
unknownContextCritical: false
},
plugins: [new EnvironmentPlugin("BBB", "CCC")]
}
] Seems like this starts multiple compilations concurrently, in fact running this configuration produces every time a different output, sometimes it prepends I'm going to investigate this tomorrow, any help/suggestion is welcome 👍 @evenstensberg could you explain what do you mean? |
similar issue resolved with quick json conversions. |
@misterdev Did you find a solution to solve this problem ? |
Currently, the
EnvironmentPlugin
is just a shortcut to use theDefinePlugin
onprocess.env
keys (link).As pointed out in #5392 this doesn't work for object destructuring:
The problem is that the
DefinePlugin
can't detect some edge case like:To solve this I add some code at the beginning of the output, that initializes the environment variables. For example, the result would be
I've tried different solutions but removing the
DefinePlugin
seems the cleanest way of solving this.This is my first contribution, every feedback is welcome 😇
What kind of change does this PR introduce?
This is a bugfix 🐛
Did you add tests for your changes?
If this seems the correct approach to solve the issue I'm going to add some tests
Does this PR introduce a breaking change?
I don't think so, now it actually adds some keys to the
process.env
as you would expect.What needs to be documented once your changes are merged?
I need to update the EnvironmentPlugin documentation