From 6afe1d3be41e191aa7c4865919d092d952e98605 Mon Sep 17 00:00:00 2001 From: Alexander Akait <4567934+alexander-akait@users.noreply.github.com> Date: Sun, 7 May 2023 03:58:40 +0300 Subject: [PATCH] perf: simplify logic, reduce extra loops and perf (#3767) --- packages/serve/src/index.ts | 4 +- packages/webpack-cli/src/types.ts | 8 +- packages/webpack-cli/src/webpack-cli.ts | 202 +++++++++--------- ...rve-basic.test.js.snap.devServer4.webpack5 | 2 +- test/watch/bail/bail.test.js | 12 +- test/watch/basic/basic.test.js | 138 ++++++++++-- test/watch/basic/no-watch.config.js | 3 + 7 files changed, 225 insertions(+), 144 deletions(-) create mode 100644 test/watch/basic/no-watch.config.js diff --git a/packages/serve/src/index.ts b/packages/serve/src/index.ts index 56f43ee3432..28374fa9194 100644 --- a/packages/serve/src/index.ts +++ b/packages/serve/src/index.ts @@ -193,7 +193,7 @@ class ServeCommand { for (const path in problemsByPath) { const problems = problemsByPath[path]; - problems.forEach((problem: Problem) => { + for (const problem of problems) { cli.logger.error( `${cli.capitalizeFirstLetter(problem.type.replace(/-/g, " "))}${ problem.value ? ` '${problem.value}'` : "" @@ -205,7 +205,7 @@ class ServeCommand { if (problem.expected) { cli.logger.error(`Expected: '${problem.expected}'`); } - }); + } } process.exit(2); diff --git a/packages/webpack-cli/src/types.ts b/packages/webpack-cli/src/types.ts index ec31fa9c364..67b65c32074 100644 --- a/packages/webpack-cli/src/types.ts +++ b/packages/webpack-cli/src/types.ts @@ -100,7 +100,6 @@ interface WebpackCLIConfig { interface WebpackCLICommand extends Command { pkg: string | undefined; forHelp: boolean | undefined; - options: WebpackCLICommandOption[]; _args: WebpackCLICommandOption[]; } @@ -187,10 +186,7 @@ type Callback = (...args: T) => void; /** * Webpack */ -type WebpackConfiguration = Configuration & { - // TODO add extends to webpack types - extends?: string | string[]; -}; +type WebpackConfiguration = Configuration; type ConfigOptions = PotentialPromise; type CallableOption = (env: Env | undefined, argv: Argv) => WebpackConfiguration; type WebpackCompiler = Compiler | MultiCompiler; @@ -209,7 +205,6 @@ type FileSystemCacheOptions = WebpackConfiguration & { type ProcessedArguments = Record; -type MultipleCompilerStatsOptions = StatsOptions & { children: StatsOptions[] }; type CommandAction = Parameters[0]; interface WebpackRunOptions extends WebpackOptionsNormalized { @@ -339,7 +334,6 @@ export { Instantiable, JsonExt, ModuleName, - MultipleCompilerStatsOptions, PackageInstallOptions, PackageManager, Path, diff --git a/packages/webpack-cli/src/webpack-cli.ts b/packages/webpack-cli/src/webpack-cli.ts index d69e845c54e..7c57942e0e9 100644 --- a/packages/webpack-cli/src/webpack-cli.ts +++ b/packages/webpack-cli/src/webpack-cli.ts @@ -475,7 +475,6 @@ class WebpackCLI implements IWebpackCLI { } const command = this.program.command(commandOptions.name, { - noHelp: commandOptions.noHelp, hidden: commandOptions.hidden, isDefault: commandOptions.isDefault, }) as WebpackCLICommand; @@ -562,9 +561,9 @@ class WebpackCLI implements IWebpackCLI { } } - options.forEach((optionForCommand) => { - this.makeOption(command, optionForCommand); - }); + for (const option of options) { + this.makeOption(command, option); + } } command.action(action); @@ -586,7 +585,7 @@ class WebpackCLI implements IWebpackCLI { let negatedDescription; const mainOptionType: WebpackCLIMainOption["type"] = new Set(); - option.configs.forEach((config) => { + for (const config of option.configs) { switch (config.type) { case "reset": mainOptionType.add(Boolean); @@ -610,7 +609,7 @@ class WebpackCLI implements IWebpackCLI { case "enum": { let hasFalseEnum = false; - const enumTypes = (config.values || []).map((value) => { + for (const value of config.values || []) { switch (typeof value) { case "string": mainOptionType.add(String); @@ -627,17 +626,15 @@ class WebpackCLI implements IWebpackCLI { mainOptionType.add(Boolean); break; } - }); + } if (!needNegativeOption) { needNegativeOption = hasFalseEnum; negatedDescription = config.negatedDescription; } - - return enumTypes; } } - }); + } mainOption = { flags: option.alias ? `-${option.alias}, --${option.name}` : `--${option.name}`, @@ -1147,9 +1144,7 @@ class WebpackCLI implements IWebpackCLI { async () => { this.webpack = await this.loadWebpack(); - return isWatchCommandUsed - ? this.getBuiltInOptions().filter((option) => option.name !== "watch") - : this.getBuiltInOptions(); + return this.getBuiltInOptions(); }, async (entries, options) => { if (entries.length > 0) { @@ -1270,11 +1265,11 @@ class WebpackCLI implements IWebpackCLI { const levenshtein = require("fastest-levenshtein"); - (command as WebpackCLICommand).options.forEach((option) => { + for (const option of (command as WebpackCLICommand).options) { if (!option.hidden && levenshtein.distance(name, option.long?.slice(2)) < 3) { this.logger.error(`Did you mean '--${option.name()}'?`); } - }); + } } } } @@ -1360,7 +1355,7 @@ class WebpackCLI implements IWebpackCLI { // Support multiple aliases subcommandTerm: (command: WebpackCLICommand) => { const humanReadableArgumentName = (argument: WebpackCLICommandOption) => { - const nameOutput = argument.name() + (argument.variadic === true ? "..." : ""); + const nameOutput = argument.name() + (argument.variadic ? "..." : ""); return argument.required ? "<" + nameOutput + ">" : "[" + nameOutput + "]"; }; @@ -1380,6 +1375,14 @@ class WebpackCLI implements IWebpackCLI { return false; } + // Hide `--watch` option when developer use `webpack watch --help` + if ( + (options[0] === "w" || options[0] === "watch") && + (option.name() === "watch" || option.name() === "no-watch") + ) { + return false; + } + switch (option.helpLevel) { case "verbose": return isVerbose; @@ -1746,9 +1749,9 @@ class WebpackCLI implements IWebpackCLI { if ((error as RechoirError)?.failures) { this.logger.error(`Unable load '${configPath}'`); this.logger.error((error as RechoirError).message); - (error as RechoirError).failures.forEach((failure) => { + for (const failure of (error as RechoirError).failures) { this.logger.error(failure.error.message); - }); + } this.logger.error("Please install one of them"); process.exit(2); } @@ -1829,7 +1832,6 @@ class WebpackCLI implements IWebpackCLI { return { options, path: configPath }; }; - // TODO better name and better type const config: WebpackCLIConfig = { options: {} as WebpackConfiguration, path: new WeakMap(), @@ -1856,18 +1858,18 @@ class WebpackCLI implements IWebpackCLI { } if (isArray) { - (loadedConfig.options as ConfigOptions[]).forEach((item) => { + for (const item of loadedConfig.options as ConfigOptions[]) { (config.options as ConfigOptions[]).push(item); - }); + } } else { config.options.push(loadedConfig.options as WebpackConfiguration); } } if (isArray) { - (loadedConfig.options as ConfigOptions[]).forEach((options) => { + for (const options of loadedConfig.options as ConfigOptions[]) { config.path.set(options, [loadedConfig.path]); - }); + } } else { config.path.set(loadedConfig.options, [loadedConfig.path]); } @@ -1875,20 +1877,22 @@ class WebpackCLI implements IWebpackCLI { config.options = config.options.length === 1 ? config.options[0] : config.options; } else { + // TODO ".mts" is not supported by `interpret`, need to add it + // Prioritize popular extensions first to avoid unnecessary fs calls + const extensions = [ + ".js", + ".mjs", + ".cjs", + ".ts", + ".cts", + ...Object.keys(interpret.extensions), + ]; // Order defines the priority, in decreasing order const defaultConfigFiles = [ "webpack.config", ".webpack/webpack.config", ".webpack/webpackfile", - ] - .map((filename) => - // Prioritize popular extensions first to avoid unnecessary fs calls - // TODO ".mts" is not supported by `interpret`, need to add it - [".js", ".mjs", ".cjs", ".ts", ".cts", ...Object.keys(interpret.extensions)].map((ext) => - path.resolve(filename + ext), - ), - ) - .reduce((accumulator, currentValue) => accumulator.concat(currentValue), []); + ].flatMap((filename) => extensions.map((ext) => path.resolve(filename + ext))); let foundDefaultConfigFile; @@ -1907,9 +1911,9 @@ class WebpackCLI implements IWebpackCLI { config.options = loadedConfig.options as WebpackConfiguration[]; if (Array.isArray(config.options)) { - config.options.forEach((item) => { + for (const item of config.options) { config.path.set(item, [loadedConfig.path]); - }); + } } else { config.path.set(loadedConfig.options, [loadedConfig.path]); } @@ -2064,21 +2068,6 @@ class WebpackCLI implements IWebpackCLI { config: WebpackCLIConfig, options: Partial, ): Promise { - const runFunctionOnEachConfig = ( - options: ConfigOptions | ConfigOptions[], - fn: CallableFunction, - ) => { - if (Array.isArray(options)) { - for (let item of options) { - item = fn(item); - } - } else { - options = fn(options); - } - - return options; - }; - if (options.analyze) { if (!this.checkPackageExists("webpack-bundle-analyzer")) { await this.doInstall("webpack-bundle-analyzer", { @@ -2107,32 +2096,18 @@ class WebpackCLI implements IWebpackCLI { >("./plugins/cli-plugin"); const internalBuildConfig = (item: WebpackConfiguration) => { - // Output warnings - if ( - item.watch && - options.argv && - options.argv.env && - (options.argv.env["WEBPACK_WATCH"] || options.argv.env["WEBPACK_SERVE"]) - ) { - this.logger.warn( - `No need to use the '${ - options.argv.env["WEBPACK_WATCH"] ? "watch" : "serve" - }' command together with '{ watch: true }' configuration, it does not make sense.`, - ); - - if (options.argv.env["WEBPACK_SERVE"]) { - item.watch = false; - } - } + const originalWatchValue = item.watch; // Apply options - const args: Record = this.getBuiltInOptions() - .filter((flag) => flag.group === "core") - .reduce((accumulator: Record, flag) => { - accumulator[flag.name] = flag as unknown as Argument; + const args: Record = this.getBuiltInOptions().reduce( + (accumulator: Record, flag) => { + if (flag.group === "core") { + accumulator[flag.name] = flag as unknown as Argument; + } return accumulator; - }, {}); - + }, + {}, + ); const values: ProcessedArguments = Object.keys(options).reduce( (accumulator: ProcessedArguments, name) => { if (name === "argv") { @@ -2150,38 +2125,59 @@ class WebpackCLI implements IWebpackCLI { {}, ); - const problems: Problem[] | null = this.webpack.cli.processArguments(args, item, values); + if (Object.keys(values).length > 0) { + const problems: Problem[] | null = this.webpack.cli.processArguments(args, item, values); - if (problems) { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const groupBy = (xs: Record[], key: string) => { - return xs.reduce((rv, x) => { - (rv[x[key]] = rv[x[key]] || []).push(x); + if (problems) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const groupBy = (xs: Record[], key: string) => { + return xs.reduce((rv, x) => { + (rv[x[key]] = rv[x[key]] || []).push(x); - return rv; - }, {}); - }; - const problemsByPath = groupBy(problems, "path"); + return rv; + }, {}); + }; + const problemsByPath = groupBy(problems, "path"); - for (const path in problemsByPath) { - const problems = problemsByPath[path]; + for (const path in problemsByPath) { + const problems = problemsByPath[path]; - problems.forEach((problem: Problem) => { - this.logger.error( - `${this.capitalizeFirstLetter(problem.type.replace(/-/g, " "))}${ - problem.value ? ` '${problem.value}'` : "" - } for the '--${problem.argument}' option${ - problem.index ? ` by index '${problem.index}'` : "" - }`, - ); + for (const problem of problems) { + this.logger.error( + `${this.capitalizeFirstLetter(problem.type.replace(/-/g, " "))}${ + problem.value ? ` '${problem.value}'` : "" + } for the '--${problem.argument}' option${ + problem.index ? ` by index '${problem.index}'` : "" + }`, + ); - if (problem.expected) { - this.logger.error(`Expected: '${problem.expected}'`); + if (problem.expected) { + this.logger.error(`Expected: '${problem.expected}'`); + } } - }); + } + + process.exit(2); } + } - process.exit(2); + // Output warnings + if ( + (typeof originalWatchValue !== "undefined" || + (options.argv && typeof options.argv.watch !== "undefined")) && + options.argv && + options.argv.env && + (options.argv.env["WEBPACK_WATCH"] || options.argv.env["WEBPACK_SERVE"]) + ) { + this.logger.warn( + `No need to use the '${ + options.argv.env["WEBPACK_WATCH"] ? "watch" : "serve" + }' command together with '{ watch: true | false }' or '--watch'/'--no-watch' configuration, it does not make sense.`, + ); + + if (options.argv.env["WEBPACK_SERVE"]) { + item.watch = false; + } } const isFileSystemCacheOptions = ( @@ -2206,13 +2202,13 @@ class WebpackCLI implements IWebpackCLI { } if (Array.isArray(configPath)) { - configPath.forEach((oneOfConfigPath) => { + for (const oneOfConfigPath of configPath) { ( item.cache.buildDependencies as NonNullable< FileSystemCacheOptions["cache"]["buildDependencies"] > ).defaultConfig.push(oneOfConfigPath); - }); + } } else { item.cache.buildDependencies.defaultConfig.push(configPath); } @@ -2270,11 +2266,15 @@ class WebpackCLI implements IWebpackCLI { analyze: options.analyze, }), ); - - return options; }; - runFunctionOnEachConfig(config.options, internalBuildConfig); + if (Array.isArray(config.options)) { + for (const item of config.options) { + internalBuildConfig(item); + } + } else { + internalBuildConfig(config.options); + } return config; } diff --git a/test/serve/basic/__snapshots__/serve-basic.test.js.snap.devServer4.webpack5 b/test/serve/basic/__snapshots__/serve-basic.test.js.snap.devServer4.webpack5 index 85d89e91e7d..4112d980d0b 100644 --- a/test/serve/basic/__snapshots__/serve-basic.test.js.snap.devServer4.webpack5 +++ b/test/serve/basic/__snapshots__/serve-basic.test.js.snap.devServer4.webpack5 @@ -79,7 +79,7 @@ exports[`basic serve usage should throw error when same ports in multicompiler: exports[`basic serve usage should throw error when same ports in multicompiler: stdout 1`] = `""`; exports[`basic serve usage should work and log warning on the \`watch option in a configuration: stderr 1`] = ` -"[webpack-cli] No need to use the 'serve' command together with '{ watch: true }' configuration, it does not make sense. +"[webpack-cli] No need to use the 'serve' command together with '{ watch: true | false }' or '--watch'/'--no-watch' configuration, it does not make sense. [webpack-dev-server] Project is running at: [webpack-dev-server] Loopback: http://localhost:/ [webpack-dev-server] On Your Network (IPv4): http://:/ diff --git a/test/watch/bail/bail.test.js b/test/watch/bail/bail.test.js index cbd7d3fe46d..16c919018fd 100644 --- a/test/watch/bail/bail.test.js +++ b/test/watch/bail/bail.test.js @@ -11,22 +11,14 @@ describe('"bail" option', () => { }); it('should not log warning without the "bail" option', async () => { - const { stderr, stdout } = await runWatch(__dirname, [ - "-c", - "no-bail-webpack.config.js", - "--watch", - ]); + const { stderr, stdout } = await runWatch(__dirname, ["-c", "no-bail-webpack.config.js"]); expect(stderr).toBeFalsy(); expect(stdout).toBeTruthy(); }); it('should not log warning without the "bail" option', async () => { - const { stderr, stdout } = await runWatch(__dirname, [ - "-c", - "no-bail-webpack.config.js", - "--watch", - ]); + const { stderr, stdout } = await runWatch(__dirname, ["-c", "no-bail-webpack.config.js"]); expect(stderr).toBeFalsy(); expect(stdout).toBeTruthy(); diff --git a/test/watch/basic/basic.test.js b/test/watch/basic/basic.test.js index 45668612790..56752a2a608 100644 --- a/test/watch/basic/basic.test.js +++ b/test/watch/basic/basic.test.js @@ -150,7 +150,51 @@ describe("basic", () => { const data = chunk.toString(); expect(data).toContain( - "No need to use the 'watch' command together with '{ watch: true }' configuration, it does not make sense.", + " No need to use the 'watch' command together with '{ watch: true | false }' or '--watch'/'--no-watch' configuration, it does not make sense.", + ); + }); + }); + + it("should log warning about the `watch` option in the configuration and recompile upon file change using the `watch` command #2", (done) => { + const proc = runAndGetProcess(__dirname, [ + "--watch", + "--mode", + "development", + "--config", + "./no-watch.config.js", + ]); + + let modified = false; + + proc.stdout.on("data", (chunk) => { + const data = chunk.toString(); + + if (data.includes("index.js")) { + for (const word of wordsInStatsv5) { + expect(data).toContain(word); + } + + if (!modified) { + process.nextTick(() => { + writeFileSync( + resolve(__dirname, "./src/index.js"), + `console.log('watch flag test');\n`, + ); + }); + + modified = true; + } else { + processKill(proc); + done(); + } + } + }); + + proc.stderr.on("data", (chunk) => { + const data = chunk.toString(); + + expect(data).toContain( + "No need to use the 'watch' command together with '{ watch: true | false }' or '--watch'/'--no-watch' configuration, it does not make sense.", ); }); }); @@ -177,31 +221,79 @@ describe("basic", () => { }); }); - it("should recompile upon file change using the `command` option and the `--watch` option and log warning", async () => { - const { exitCode, stderr, stdout } = await run(__dirname, [ - "watch", - "--watch", - "--mode", - "development", - ]); + it("should recompile upon file change using the `command` option and the `--watch` option and log warning", (done) => { + const proc = runAndGetProcess(__dirname, ["watch", "--watch", "--mode", "development"]); + + let modified = false; + + proc.stdout.on("data", (chunk) => { + const data = chunk.toString(); - expect(exitCode).toBe(2); - expect(stderr).toContain("Error: Unknown option '--watch'"); - expect(stderr).toContain("Run 'webpack --help' to see available commands and options"); - expect(stdout).toBeFalsy(); + if (data.includes("index.js")) { + for (const word of wordsInStatsv5) { + expect(data).toContain(word); + } + + if (!modified) { + process.nextTick(() => { + writeFileSync( + resolve(__dirname, "./src/index.js"), + `console.log('watch flag test');\n`, + ); + }); + + modified = true; + } else { + processKill(proc); + done(); + } + } + }); + + proc.stderr.on("data", (chunk) => { + const data = chunk.toString(); + + expect(data).toContain( + "No need to use the 'watch' command together with '{ watch: true | false }' or '--watch'/'--no-watch' configuration, it does not make sense.", + ); + }); }); - it("should recompile upon file change using the `command` option and the `--no-watch` option and log warning", async () => { - const { exitCode, stderr, stdout } = await run(__dirname, [ - "watch", - "--no-watch", - "--mode", - "development", - ]); + it("should recompile upon file change using the `command` option and the `--watch` option and log warning", (done) => { + const proc = runAndGetProcess(__dirname, ["watch", "--no-watch", "--mode", "development"]); - expect(exitCode).toBe(2); - expect(stderr).toContain("Error: Unknown option '--no-watch'"); - expect(stderr).toContain("Run 'webpack --help' to see available commands and options"); - expect(stdout).toBeFalsy(); + let modified = false; + + proc.stdout.on("data", (chunk) => { + const data = chunk.toString(); + + if (data.includes("index.js")) { + for (const word of wordsInStatsv5) { + expect(data).toContain(word); + } + + if (!modified) { + process.nextTick(() => { + writeFileSync( + resolve(__dirname, "./src/index.js"), + `console.log('watch flag test');\n`, + ); + }); + + modified = true; + } else { + processKill(proc); + done(); + } + } + }); + + proc.stderr.on("data", (chunk) => { + const data = chunk.toString(); + + expect(data).toContain( + "No need to use the 'watch' command together with '{ watch: true | false }' or '--watch'/'--no-watch' configuration, it does not make sense.", + ); + }); }); }); diff --git a/test/watch/basic/no-watch.config.js b/test/watch/basic/no-watch.config.js new file mode 100644 index 00000000000..aea00c7b572 --- /dev/null +++ b/test/watch/basic/no-watch.config.js @@ -0,0 +1,3 @@ +module.exports = { + watch: false, +};