From e04c56a2d72535e1892a0c92021bf334bd69a6d1 Mon Sep 17 00:00:00 2001 From: Nitin Kumar Date: Thu, 9 Jun 2022 20:06:18 +0530 Subject: [PATCH 1/4] fix: improve parsing of --env flag --- packages/webpack-cli/src/webpack-cli.ts | 17 ++++++++++------- .../function-with-env/function-with-env.test.js | 16 +++++++++++++--- .../type/function-with-env/webpack.config.js | 9 +-------- 3 files changed, 24 insertions(+), 18 deletions(-) diff --git a/packages/webpack-cli/src/webpack-cli.ts b/packages/webpack-cli/src/webpack-cli.ts index 008c30484e5..e8db99ecc47 100644 --- a/packages/webpack-cli/src/webpack-cli.ts +++ b/packages/webpack-cli/src/webpack-cli.ts @@ -756,13 +756,13 @@ class WebpackCLI implements IWebpackCLI { value: string, previous: Record = {}, ): Record => { - // for https://github.com/webpack/webpack-cli/issues/2642 - if (value.endsWith("=")) { - value.concat('""'); - } - // This ensures we're only splitting by the first `=` - const [allKeys, val] = value.split(/=(.+)/, 2); + let [allKeys, val] = value.split(/=(.+)/, 2); + if (typeof val === "undefined" && allKeys.endsWith("=")) { + // remove '=' from key + allKeys = allKeys.slice(0, -1); + val = "undefined"; + } const splitKeys = allKeys.split(/\.(?!$)/); let prevRef = previous; @@ -777,8 +777,11 @@ class WebpackCLI implements IWebpackCLI { } if (index === splitKeys.length - 1) { - if (typeof val === "string") { + if (typeof val === "string" && val !== "undefined") { prevRef[someKey] = val; + } else if (val === "undefined") { + // @ts-expect-error we explicitly want to set it to undefined + prevRef[someKey] = undefined; } else { prevRef[someKey] = true; } diff --git a/test/build/config/type/function-with-env/function-with-env.test.js b/test/build/config/type/function-with-env/function-with-env.test.js index 4e48c406639..b060f5725f4 100644 --- a/test/build/config/type/function-with-env/function-with-env.test.js +++ b/test/build/config/type/function-with-env/function-with-env.test.js @@ -142,9 +142,19 @@ describe("function configuration", () => { expect(exitCode).toBe(0); expect(stderr).toBeFalsy(); - expect(stdout).toBeTruthy(); - // Should generate the appropriate files - expect(existsSync(resolve(__dirname, "./dist/equal-at-the-end.js"))).toBeTruthy(); + // should log foo: undefined + expect(stdout).toContain("foo: undefined"); + }); + + it('Supports env variable with "=$NON_EXISTENT_VAR" at the end', async () => { + const { exitCode, stderr, stdout } = await run(__dirname, ["--env", `foo=$NON_EXISTENT_VAR`], { + shell: true, + }); + + expect(exitCode).toBe(0); + expect(stderr).toBeFalsy(); + // should log foo: undefined + expect(stdout).toContain("foo: undefined"); }); it("is able to understand multiple env flags", async () => { diff --git a/test/build/config/type/function-with-env/webpack.config.js b/test/build/config/type/function-with-env/webpack.config.js index edd85d25427..0e172f24e4a 100644 --- a/test/build/config/type/function-with-env/webpack.config.js +++ b/test/build/config/type/function-with-env/webpack.config.js @@ -1,6 +1,7 @@ const { DefinePlugin } = require("webpack"); module.exports = (env) => { + console.log(env); if (env.isProd) { return { entry: "./a.js", @@ -25,14 +26,6 @@ module.exports = (env) => { }, }; } - if (env["foo="]) { - return { - entry: "./a.js", - output: { - filename: "equal-at-the-end.js", - }, - }; - } return { entry: "./a.js", mode: "development", From b5b3c5682391d8c6181ca6eff2f7d13b32357243 Mon Sep 17 00:00:00 2001 From: Nitin Kumar Date: Thu, 9 Jun 2022 20:47:30 +0530 Subject: [PATCH 2/4] fix: improve parsing of `env` flag --- packages/webpack-cli/src/webpack-cli.ts | 21 ++++++++++--------- .../function-with-env.test.js | 16 +++++++++++++- .../type/function-with-env/webpack.config.js | 8 +++++++ 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/packages/webpack-cli/src/webpack-cli.ts b/packages/webpack-cli/src/webpack-cli.ts index e8db99ecc47..9a0ffeef947 100644 --- a/packages/webpack-cli/src/webpack-cli.ts +++ b/packages/webpack-cli/src/webpack-cli.ts @@ -757,17 +757,21 @@ class WebpackCLI implements IWebpackCLI { previous: Record = {}, ): Record => { // This ensures we're only splitting by the first `=` - let [allKeys, val] = value.split(/=(.+)/, 2); - if (typeof val === "undefined" && allKeys.endsWith("=")) { - // remove '=' from key - allKeys = allKeys.slice(0, -1); - val = "undefined"; - } + const [allKeys, val] = value.split(/=(.+)/, 2); const splitKeys = allKeys.split(/\.(?!$)/); let prevRef = previous; splitKeys.forEach((someKey, index) => { + // https://github.com/webpack/webpack-cli/issues/3284 + if (someKey.endsWith("=")) { + // remove '=' from key + someKey = someKey.slice(0, -1); + // @ts-expect-error we explicitly want to set it to undefined + prevRef[someKey] = undefined; + return; + } + if (!prevRef[someKey]) { prevRef[someKey] = {}; } @@ -777,11 +781,8 @@ class WebpackCLI implements IWebpackCLI { } if (index === splitKeys.length - 1) { - if (typeof val === "string" && val !== "undefined") { + if (typeof val === "string") { prevRef[someKey] = val; - } else if (val === "undefined") { - // @ts-expect-error we explicitly want to set it to undefined - prevRef[someKey] = undefined; } else { prevRef[someKey] = true; } diff --git a/test/build/config/type/function-with-env/function-with-env.test.js b/test/build/config/type/function-with-env/function-with-env.test.js index b060f5725f4..71703a5997a 100644 --- a/test/build/config/type/function-with-env/function-with-env.test.js +++ b/test/build/config/type/function-with-env/function-with-env.test.js @@ -1,7 +1,7 @@ "use strict"; const { existsSync } = require("fs"); const { resolve } = require("path"); -const { run, readFile } = require("../../../../utils/test-utils"); +const { run, readFile, isWindows } = require("../../../../utils/test-utils"); describe("function configuration", () => { it("should throw when env is not supplied", async () => { @@ -157,6 +157,20 @@ describe("function configuration", () => { expect(stdout).toContain("foo: undefined"); }); + // macOS/Linux specific syntax + if (!isWindows) { + it('Supports env variable with "foo=undefined" at the end', async () => { + const { exitCode, stderr, stdout } = await run(__dirname, ["--env", `foo=undefined`]); + + expect(exitCode).toBe(0); + expect(stderr).toBeFalsy(); + // should log foo: 'undefined' + expect(stdout).toContain("foo: 'undefined'"); + // Should generate the appropriate files + expect(existsSync(resolve(__dirname, "./dist/undefined-foo.js"))).toBeTruthy(); + }); + } + it("is able to understand multiple env flags", async () => { const { exitCode, stderr, stdout } = await run(__dirname, [ "--env", diff --git a/test/build/config/type/function-with-env/webpack.config.js b/test/build/config/type/function-with-env/webpack.config.js index 0e172f24e4a..afbfc55dfd1 100644 --- a/test/build/config/type/function-with-env/webpack.config.js +++ b/test/build/config/type/function-with-env/webpack.config.js @@ -26,6 +26,14 @@ module.exports = (env) => { }, }; } + if (env.foo === "undefined") { + return { + entry: "./a.js", + output: { + filename: "undefined-foo.js", + }, + }; + } return { entry: "./a.js", mode: "development", From 07614e9d6b616f2801d4ecd7fe9c4dd10908086b Mon Sep 17 00:00:00 2001 From: Nitin Kumar Date: Fri, 10 Jun 2022 06:37:43 +0530 Subject: [PATCH 3/4] test: add more cases --- .../function-with-env.test.js | 72 +++++++++++++------ .../function-with-env/webpack.env.config.js | 1 + 2 files changed, 52 insertions(+), 21 deletions(-) diff --git a/test/build/config/type/function-with-env/function-with-env.test.js b/test/build/config/type/function-with-env/function-with-env.test.js index 71703a5997a..d5dacd4d18c 100644 --- a/test/build/config/type/function-with-env/function-with-env.test.js +++ b/test/build/config/type/function-with-env/function-with-env.test.js @@ -17,7 +17,7 @@ describe("function configuration", () => { expect(exitCode).toBe(0); expect(stderr).toBeFalsy(); - expect(stdout).toBeTruthy(); + expect(stdout).toContain("isProd: true"); // Should generate the appropriate files expect(existsSync(resolve(__dirname, "./dist/prod.js"))).toBeTruthy(); }); @@ -27,7 +27,7 @@ describe("function configuration", () => { expect(exitCode).toBe(0); expect(stderr).toBeFalsy(); - expect(stdout).toBeTruthy(); + expect(stdout).toContain("isDev: true"); // Should generate the appropriate files expect(existsSync(resolve(__dirname, "./dist/dev.js"))).toBeTruthy(); }); @@ -44,7 +44,8 @@ describe("function configuration", () => { expect(exitCode).toBe(0); expect(stderr).toBeFalsy(); - expect(stdout).toBeTruthy(); + expect(stdout).toContain("environment: 'production'"); + expect(stdout).toContain("app: { title: 'Luffy' }"); // Should generate the appropriate files expect(existsSync(resolve(__dirname, "./dist/Luffy.js"))).toBeTruthy(); }); @@ -62,6 +63,7 @@ describe("function configuration", () => { expect(exitCode).toBe(0); expect(stderr).toBeFalsy(); expect(stdout).toBeTruthy(); + expect(stdout).toContain("environment: 'production'"); // Should generate the appropriate files expect(existsSync(resolve(__dirname, "./dist/Atsumu.js"))).toBeTruthy(); }); @@ -78,7 +80,8 @@ describe("function configuration", () => { expect(exitCode).toBe(0); expect(stderr).toBeFalsy(); - expect(stdout).toBeTruthy(); + expect(stdout).toContain("environment: 'multipleq'"); + expect(stdout).toContain("file: 'name=is=Eren'"); // Should generate the appropriate files expect(existsSync(resolve(__dirname, "./dist/name=is=Eren.js"))).toBeTruthy(); }); @@ -95,7 +98,8 @@ describe("function configuration", () => { expect(exitCode).toBe(0); expect(stderr).toBeFalsy(); - expect(stdout).toBeTruthy(); + expect(stdout).toContain("environment: 'dot'"); + expect(stdout).toContain("'name.': 'Hisoka'"); // Should generate the appropriate files expect(existsSync(resolve(__dirname, "./dist/Hisoka.js"))).toBeTruthy(); }); @@ -112,7 +116,8 @@ describe("function configuration", () => { expect(exitCode).toBe(0); expect(stderr).toBeFalsy(); - expect(stdout).toBeTruthy(); + expect(stdout).toContain("environment: 'dot'"); + expect(stdout).toContain("'name.': true"); // Should generate the appropriate files expect(existsSync(resolve(__dirname, "./dist/true.js"))).toBeTruthy(); }); @@ -122,7 +127,7 @@ describe("function configuration", () => { expect(exitCode).toBe(0); expect(stderr).toBeFalsy(); - expect(stdout).toBeTruthy(); + expect(stdout).toContain(`foo: "''"`); // Should generate the appropriate files expect(existsSync(resolve(__dirname, "./dist/empty-string.js"))).toBeTruthy(); }); @@ -132,7 +137,7 @@ describe("function configuration", () => { expect(exitCode).toBe(0); expect(stderr).toBeFalsy(); - expect(stdout).toBeTruthy(); + expect(stdout).toContain(`foo: "bar=''"`); // Should generate the appropriate files expect(existsSync(resolve(__dirname, "./dist/new-empty-string.js"))).toBeTruthy(); }); @@ -146,28 +151,52 @@ describe("function configuration", () => { expect(stdout).toContain("foo: undefined"); }); - it('Supports env variable with "=$NON_EXISTENT_VAR" at the end', async () => { - const { exitCode, stderr, stdout } = await run(__dirname, ["--env", `foo=$NON_EXISTENT_VAR`], { - shell: true, - }); + it('Supports env variable with "foo=undefined" at the end', async () => { + const { exitCode, stderr, stdout } = await run(__dirname, ["--env", `foo=undefined`]); expect(exitCode).toBe(0); expect(stderr).toBeFalsy(); - // should log foo: undefined - expect(stdout).toContain("foo: undefined"); + // should log foo: 'undefined' + expect(stdout).toContain("foo: 'undefined'"); + // Should generate the appropriate files + expect(existsSync(resolve(__dirname, "./dist/undefined-foo.js"))).toBeTruthy(); }); // macOS/Linux specific syntax if (!isWindows) { - it('Supports env variable with "foo=undefined" at the end', async () => { - const { exitCode, stderr, stdout } = await run(__dirname, ["--env", `foo=undefined`]); + it("Supports empty string in shell environment", async () => { + const { exitCode, stderr, stdout } = await run(__dirname, ["--env", "foo=\\'\\'"], { + shell: true, + }); expect(exitCode).toBe(0); expect(stderr).toBeFalsy(); - // should log foo: 'undefined' - expect(stdout).toContain("foo: 'undefined'"); + expect(stdout).toContain(`foo: "''"`); // Should generate the appropriate files - expect(existsSync(resolve(__dirname, "./dist/undefined-foo.js"))).toBeTruthy(); + expect(existsSync(resolve(__dirname, "./dist/empty-string.js"))).toBeTruthy(); + }); + it("should set the variable to undefined if empty string is not escaped in shell environment", async () => { + const { exitCode, stderr, stdout } = await run(__dirname, ["--env", "foo=''"], { + shell: true, + }); + + expect(exitCode).toBe(0); + expect(stderr).toBeFalsy(); + expect(stdout).toContain(`foo: undefined`); + }); + it('Supports env variable with "=$NON_EXISTENT_VAR" at the end', async () => { + const { exitCode, stderr, stdout } = await run( + __dirname, + ["--env", `foo=$NON_EXISTENT_VAR`], + { + shell: true, + }, + ); + + expect(exitCode).toBe(0); + expect(stderr).toBeFalsy(); + // should log foo: undefined + expect(stdout).toContain("foo: undefined"); }); } @@ -183,7 +212,8 @@ describe("function configuration", () => { expect(exitCode).toBe(0); expect(stderr).toBeFalsy(); - expect(stdout).toBeTruthy(); + expect(stdout).toContain("verboseStats: true"); + expect(stdout).toContain("envMessage: true"); // check that the verbose env is respected expect(stdout).toContain("LOG from webpack"); @@ -213,7 +243,7 @@ describe("function configuration", () => { expect(exitCode).toBe(0); expect(stderr).toBeFalsy(); - expect(stdout).toBeTruthy(); + expect(stdout).toContain("'name.': 'baz'"); // Should generate the appropriate files expect(existsSync(resolve(__dirname, "./dist/baz.js"))).toBeTruthy(); }); diff --git a/test/build/config/type/function-with-env/webpack.env.config.js b/test/build/config/type/function-with-env/webpack.env.config.js index 531aa2a6352..8d9261b3892 100644 --- a/test/build/config/type/function-with-env/webpack.env.config.js +++ b/test/build/config/type/function-with-env/webpack.env.config.js @@ -1,4 +1,5 @@ module.exports = (env) => { + console.log(env); const { environment, app, file } = env; const customName = file && file.name && file.name.is && file.name.is.this; const appTitle = app && app.title; From c43fa6d81100f7f413ef60daff410437c845883c Mon Sep 17 00:00:00 2001 From: Nitin Kumar Date: Fri, 10 Jun 2022 09:14:08 +0530 Subject: [PATCH 4/4] chore: remove redundant check Co-authored-by: Rishabh Chawla --- .../config/type/function-with-env/function-with-env.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/build/config/type/function-with-env/function-with-env.test.js b/test/build/config/type/function-with-env/function-with-env.test.js index d5dacd4d18c..b022b7ac572 100644 --- a/test/build/config/type/function-with-env/function-with-env.test.js +++ b/test/build/config/type/function-with-env/function-with-env.test.js @@ -62,7 +62,6 @@ describe("function configuration", () => { expect(exitCode).toBe(0); expect(stderr).toBeFalsy(); - expect(stdout).toBeTruthy(); expect(stdout).toContain("environment: 'production'"); // Should generate the appropriate files expect(existsSync(resolve(__dirname, "./dist/Atsumu.js"))).toBeTruthy();