webpack / webpack.js.org

Repository for webpack documentation and more!
https://webpack.js.org
Creative Commons Attribution 4.0 International
2.21k stars 3.31k forks source link

EnvironmentPlugin doesn't error if environment variable wasn't found? #7448

Closed fasiha closed 1 week ago

fasiha commented 2 weeks ago

Bug report

What is the current behavior?

Webpack configured to use EnvironmentPlugin will happily build code with unknown environment variables like process.env.UNKNOWN.

If the current behavior is a bug, please provide the steps to reproduce.

I have created a fork of the webpack repo and tweaked an example to demonstrate the issue at https://github.com/fasiha/webpack/tree/example-environment-plugin. You can either check out that fork and branch and then run

yarn && yarn setup && yarn add --dev webpack-cli
cd examples/multi-compiler
node build.js

or simply check out the current main branch and apply the following patch to modify one of the examples:

diff --git a/examples/multi-compiler/example.js b/examples/multi-compiler/example.js
index 7a10193a2..ac996212d 100644
--- a/examples/multi-compiler/example.js
+++ b/examples/multi-compiler/example.js
@@ -1,4 +1,4 @@
-if(ENV === "mobile") {
+if (process.env.TESTING && process.env.ENV === "mobile") {
    require("./mobile-stuff");
 }
-console.log("Running " + ENV + " build");
\ No newline at end of file
+console.log("Running " + process.env.ENV + " build");
diff --git a/examples/multi-compiler/webpack.config.js b/examples/multi-compiler/webpack.config.js
index e7b01428c..0de1ae133 100644
--- a/examples/multi-compiler/webpack.config.js
+++ b/examples/multi-compiler/webpack.config.js
@@ -11,8 +11,8 @@ module.exports = [
            filename: "mobile.js"
        },
        plugins: [
-           new webpack.DefinePlugin({
-               ENV: JSON.stringify("mobile")
+           new webpack.EnvironmentPlugin({
+               ENV: "mobile"
            })
        ]
    },
@@ -26,8 +26,8 @@ module.exports = [
            filename: "desktop.js"
        },
        plugins: [
-           new webpack.DefinePlugin({
-               ENV: JSON.stringify("desktop")
+           new webpack.EnvironmentPlugin({
+               ENV: "desktop"
            })
        ]
    }

Then run cd examples/multi-compiler && node build.js.

Whether you use my branch or the above patch, building the modified multi-compiler example will happily run without any errors.

The resulting code has process.env.ENV correctly interpolated to desktop or mobile per Webpack config, but the new unknown process.env.TESTING is erroneously left intact.

What is the expected behavior?

I expect building multi-compiler in the above branch (or the current webpack main with the patch applied) to fail per EnvironmentPlugin docs:

If an environment variable is not found during bundling and no default value was provided, webpack will throw an error instead of a warning.

Other relevant information: webpack version: main branch 2e174de66 Node.js version: v20.18.0 Operating System: macOS and Rocky9 Linux Additional tools:

alexander-akait commented 2 weeks ago

If an environment variable is not found during bundling and no default value was provided, webpack will throw an error instead of a warning.

This is a documentation issue.

It means we use this code to get value:

const value =
  process.env[key] !== undefined
    ? process.env[key]
    : this.defaultValues[key];

And if you have new webpack.EnvironmentPlugin(["ENV"]), it will throw an error when you don't have it in process.env, we never threw an error if we found the value in the source code.

I would recommend using a linter in this case if you want to prohibit the use of some values in the source code (in process.env).

In your case you can use

new webpack.EnvironmentPlugin(["ENV", "TESTING"])

or


new webpack.EnvironmentPlugin({
  ENV: "desktop",
  TESTING: undefined
})`  

by using the object you provide default values
fasiha commented 2 weeks ago

Understood, thanks, sorry for misunderstanding! It was very surprising to see process.env.… in the built output but I now see that this is intended behavior.

If it's ok with you, I'll prepare a PR explaining this a bit more in the documentation?

alexander-akait commented 2 weeks ago

@fasiha Yeah, feel free to send a pr :heart: