From 402c0fe9d4c09e75b9abec3bf44df430f4b62dff Mon Sep 17 00:00:00 2001 From: Nitin Kumar Date: Fri, 10 Jun 2022 17:59:33 +0530 Subject: [PATCH] fix: improve parsing of `--env` flag (#3286) --- packages/webpack-cli/src/webpack-cli.ts | 14 ++-- .../function-with-env.test.js | 81 +++++++++++++++---- .../type/function-with-env/webpack.config.js | 5 +- .../function-with-env/webpack.env.config.js | 1 + 4 files changed, 80 insertions(+), 21 deletions(-) diff --git a/packages/webpack-cli/src/webpack-cli.ts b/packages/webpack-cli/src/webpack-cli.ts index 008c30484e5..9a0ffeef947 100644 --- a/packages/webpack-cli/src/webpack-cli.ts +++ b/packages/webpack-cli/src/webpack-cli.ts @@ -756,11 +756,6 @@ 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); const splitKeys = allKeys.split(/\.(?!$)/); @@ -768,6 +763,15 @@ class WebpackCLI implements IWebpackCLI { 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] = {}; } 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..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 @@ -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 () => { @@ -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(); }); @@ -61,7 +62,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 +79,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 +97,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 +115,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 +126,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 +136,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(); }); @@ -142,11 +146,59 @@ describe("function configuration", () => { expect(exitCode).toBe(0); expect(stderr).toBeFalsy(); - expect(stdout).toBeTruthy(); + // should log foo: undefined + expect(stdout).toContain("foo: undefined"); + }); + + 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/equal-at-the-end.js"))).toBeTruthy(); + expect(existsSync(resolve(__dirname, "./dist/undefined-foo.js"))).toBeTruthy(); }); + // macOS/Linux specific syntax + if (!isWindows) { + 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(); + expect(stdout).toContain(`foo: "''"`); + // Should generate the appropriate files + 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"); + }); + } + it("is able to understand multiple env flags", async () => { const { exitCode, stderr, stdout } = await run(__dirname, [ "--env", @@ -159,7 +211,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"); @@ -189,7 +242,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.config.js b/test/build/config/type/function-with-env/webpack.config.js index edd85d25427..afbfc55dfd1 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,11 +26,11 @@ module.exports = (env) => { }, }; } - if (env["foo="]) { + if (env.foo === "undefined") { return { entry: "./a.js", output: { - filename: "equal-at-the-end.js", + filename: "undefined-foo.js", }, }; } 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;