From e06392ae531d111dab9603c785001338740f42ab Mon Sep 17 00:00:00 2001 From: Anshuman Verma Date: Mon, 17 Aug 2020 17:20:37 +0530 Subject: [PATCH] perf: do not spawn new process for running webpack (#1741) --- packages/webpack-cli/README.md | 1 - .../__tests__/cli-executer.test.js | 2 +- packages/webpack-cli/bin/cli.js | 6 +- packages/webpack-cli/lib/bootstrap.js | 31 ++++---- .../webpack-cli/lib/utils/cli-executer.js | 4 +- packages/webpack-cli/lib/utils/cli-flags.js | 8 --- .../webpack-cli/lib/utils/parse-node-args.js | 27 ------- packages/webpack-cli/lib/webpack-cli.js | 4 +- test/node/node.test.js | 70 +++++-------------- test/utils/test-utils.js | 10 +-- 10 files changed, 46 insertions(+), 117 deletions(-) delete mode 100644 packages/webpack-cli/lib/utils/parse-node-args.js diff --git a/packages/webpack-cli/README.md b/packages/webpack-cli/README.md index 673d751e578..4751e251fec 100644 --- a/packages/webpack-cli/README.md +++ b/packages/webpack-cli/README.md @@ -54,7 +54,6 @@ yarn add webpack-cli --dev -j, --json Prints result as JSON --mode string Defines the mode to pass to webpack -v, --version Get current version - --node-args string[] NodeJS flags --stats string It instructs webpack on how to treat the stats --no-stats Disables stats output ``` diff --git a/packages/webpack-cli/__tests__/cli-executer.test.js b/packages/webpack-cli/__tests__/cli-executer.test.js index fdd12f9de71..a67b63bc293 100644 --- a/packages/webpack-cli/__tests__/cli-executer.test.js +++ b/packages/webpack-cli/__tests__/cli-executer.test.js @@ -47,7 +47,7 @@ describe('CLI Executer', () => { it('runs enquirer options then runs webpack', async () => { await cliExecuter(); - // ensure that the webpack runner is called + // ensure that the webpack runCLI is called expect(runner.mock.calls.length).toEqual(1); expect(runner.mock.calls[0]).toEqual([[], ['--config', 'test1', '--entry', 'test2', '--progress']]); diff --git a/packages/webpack-cli/bin/cli.js b/packages/webpack-cli/bin/cli.js index 34faad08445..960fc1fad39 100755 --- a/packages/webpack-cli/bin/cli.js +++ b/packages/webpack-cli/bin/cli.js @@ -3,8 +3,7 @@ 'use strict'; require('v8-compile-cache'); const importLocal = require('import-local'); -const parseNodeArgs = require('../lib/utils/parse-node-args'); -const runner = require('../lib/runner'); +const runCLI = require('../lib/bootstrap'); // Prefer the local installation of webpack-cli if (importLocal(__filename)) { @@ -13,6 +12,5 @@ if (importLocal(__filename)) { process.title = 'webpack'; const [, , ...rawArgs] = process.argv; -const { cliArgs, nodeArgs } = parseNodeArgs(rawArgs); -runner(nodeArgs, cliArgs); +runCLI(rawArgs); diff --git a/packages/webpack-cli/lib/bootstrap.js b/packages/webpack-cli/lib/bootstrap.js index b7bbc6ac2d4..b95fb54c629 100644 --- a/packages/webpack-cli/lib/bootstrap.js +++ b/packages/webpack-cli/lib/bootstrap.js @@ -7,20 +7,25 @@ const argParser = require('./utils/arg-parser'); require('./utils/process-log'); process.title = 'webpack-cli'; -const isCommandUsed = (commands) => +// Create a new instance of the CLI object +const cli = new WebpackCLI(); + +const isCommandUsed = (args) => commands.find((cmd) => { - return process.argv.includes(cmd.name) || process.argv.includes(cmd.alias); + return args.includes(cmd.name) || args.includes(cmd.alias); }); -async function runCLI(cli, commandIsUsed) { +async function runCLI(cliArgs) { let args; + + const commandIsUsed = isCommandUsed(cliArgs); const runVersion = () => { - cli.runVersion(process.argv, commandIsUsed); + cli.runVersion(cliArgs, commandIsUsed); }; - const parsedArgs = argParser(core, process.argv, false, process.title, cli.runHelp, runVersion, commands); + const parsedArgs = argParser(core, cliArgs, true, process.title, cli.runHelp, runVersion, commands); if (parsedArgs.unknownArgs.includes('help')) { - cli.runHelp(process.argv); + cli.runHelp(cliArgs); process.exit(0); } @@ -55,7 +60,7 @@ async function runCLI(cli, commandIsUsed) { parsedArgs.unknownArgs.forEach((unknown) => { logger.warn(`Unknown argument: ${unknown}`); }); - cliExecuter(); + await cliExecuter(); return; } const parsedArgsOpts = parsedArgs.opts; @@ -77,10 +82,10 @@ async function runCLI(cli, commandIsUsed) { } else if (err.name === 'ALREADY_SET') { const argsMap = {}; const keysToDelete = []; - process.argv.forEach((arg, idx) => { + cliArgs.forEach((arg, idx) => { const oldMapValue = argsMap[arg]; argsMap[arg] = { - value: process.argv[idx], + value: cliArgs[idx], pos: idx, }; // Swap idx of overridden value @@ -92,8 +97,8 @@ async function runCLI(cli, commandIsUsed) { // Filter out the value for the overridden key const newArgKeys = Object.keys(argsMap).filter((arg) => !keysToDelete.includes(argsMap[arg].pos)); // eslint-disable-next-line require-atomic-updates - process.argv = newArgKeys; - args = argParser('', core, process.argv); + cliArgs = newArgKeys; + args = argParser('', core, cliArgs); await cli.run(args.opts, core); process.stdout.write('\n'); logger.warn('Duplicate flags found, defaulting to last set value'); @@ -104,6 +109,4 @@ async function runCLI(cli, commandIsUsed) { } } -const commandIsUsed = isCommandUsed(commands); -const cli = new WebpackCLI(); -runCLI(cli, commandIsUsed); +module.exports = runCLI; diff --git a/packages/webpack-cli/lib/utils/cli-executer.js b/packages/webpack-cli/lib/utils/cli-executer.js index bddb21d8cdb..fbb2ef6895c 100644 --- a/packages/webpack-cli/lib/utils/cli-executer.js +++ b/packages/webpack-cli/lib/utils/cli-executer.js @@ -1,8 +1,8 @@ const { MultiSelect, Input } = require('enquirer'); const { cyan } = require('colorette'); -const runner = require('../runner'); const logger = require('./logger'); const cliArgs = require('./cli-flags').core; +const runner = require('../runner'); async function prompter() { const args = []; @@ -56,7 +56,7 @@ async function run() { const args = await prompter(); process.stdout.write('\n'); logger.info('Executing CLI\n'); - runner([], args); + await runner([], args); } catch (err) { logger.error(`Action Interrupted, use ${cyan('webpack-cli help')} to see possible options.`); } diff --git a/packages/webpack-cli/lib/utils/cli-flags.js b/packages/webpack-cli/lib/utils/cli-flags.js index 7477a0b5da7..b11a1debb08 100644 --- a/packages/webpack-cli/lib/utils/cli-flags.js +++ b/packages/webpack-cli/lib/utils/cli-flags.js @@ -236,14 +236,6 @@ module.exports = { group: HELP_GROUP, description: 'Get current version', }, - { - name: 'node-args', - usage: '--node-args "--max-old-space-size=1024"', - type: String, - multiple: true, - group: BASIC_GROUP, - description: 'NodeJS flags', - }, { name: 'stats', usage: '--stats ', diff --git a/packages/webpack-cli/lib/utils/parse-node-args.js b/packages/webpack-cli/lib/utils/parse-node-args.js deleted file mode 100644 index 9ea49b8d6a9..00000000000 --- a/packages/webpack-cli/lib/utils/parse-node-args.js +++ /dev/null @@ -1,27 +0,0 @@ -/** - * Parse cli args and split these to args for node js and the rest - * - * @param {string[]} rawArgs raw cli args - * @returns {{cliArgs: string[], nodeArgs: string[]}} cli and nodejs args - */ -module.exports = (rawArgs) => { - const cliArgs = []; - const nodeArgs = []; - let isNodeArg = false; - - for (const value of rawArgs) { - if (value === '--node-args') { - isNodeArg = true; - } else if (value.startsWith('--node-args=')) { - const [, argValue] = value.match(/^--node-args="?(.+?)"?$/); - nodeArgs.push(argValue); - } else if (isNodeArg) { - isNodeArg = false; - nodeArgs.push(...value.split(' ')); - } else { - cliArgs.push(value); - } - } - - return { cliArgs, nodeArgs }; -}; diff --git a/packages/webpack-cli/lib/webpack-cli.js b/packages/webpack-cli/lib/webpack-cli.js index aab0e10f6ab..a6f6909acf9 100644 --- a/packages/webpack-cli/lib/webpack-cli.js +++ b/packages/webpack-cli/lib/webpack-cli.js @@ -281,7 +281,7 @@ class WebpackCLI extends GroupHelper { const subject = allNames.filter((name) => { return args.includes(name); })[0]; - const invalidArgs = hasUnknownArgs(args.slice(2), ...allNames); + const invalidArgs = hasUnknownArgs(args, ...allNames); const isCommand = commands.includes(subject); options.enabled = !args.includes('--no-color'); return new HelpGroup().outputHelp(isCommand, subject, invalidArgs); @@ -291,7 +291,7 @@ class WebpackCLI extends GroupHelper { const HelpGroup = require('./groups/HelpGroup'); const { commands, allNames, hasUnknownArgs } = require('./utils/unknown-args'); const commandsUsed = args.filter((val) => commands.includes(val)); - const invalidArgs = hasUnknownArgs(args.slice(2), ...allNames); + const invalidArgs = hasUnknownArgs(args, ...allNames); options.enabled = !args.includes('--no-color'); return new HelpGroup().outputVersion(externalPkg, commandsUsed, invalidArgs); } diff --git a/test/node/node.test.js b/test/node/node.test.js index a449f4eee15..63114299a8e 100644 --- a/test/node/node.test.js +++ b/test/node/node.test.js @@ -2,57 +2,19 @@ const { stat } = require('fs'); const { resolve } = require('path'); const { run } = require('../utils/test-utils'); -const parseNodeArgs = require('../../packages/webpack-cli/lib/utils/parse-node-args'); +// TODO - We pass node args to `nodeOptions` in execa, +// passing via NODE_OPTIONS= in env in execa +// throws different error from what we manually see describe('node flags', () => { - it('parseNodeArgs helper must work correctly', () => { - [ - { - rawArgs: ['--foo', '--bar', '--baz=quux'], - expectedCliArgs: ['--foo', '--bar', '--baz=quux'], - expectedNodeArgs: [], - }, - { - rawArgs: ['--foo', '--bar', '--baz=quux', '--node-args', '--name1=value1', '--node-args', '--name2 value2'], - expectedCliArgs: ['--foo', '--bar', '--baz=quux'], - expectedNodeArgs: ['--name1=value1', '--name2', 'value2'], - }, - { - rawArgs: [ - '--node-args', - '--name1=value1', - '--node-args', - '--name2="value2"', - '--node-args', - '--name3 value3', - '--node-args', - '-k v', - ], - expectedCliArgs: [], - expectedNodeArgs: ['--name1=value1', '--name2="value2"', '--name3', 'value3', '-k', 'v'], - }, - ].map(({ rawArgs, expectedNodeArgs, expectedCliArgs }) => { - const { nodeArgs, cliArgs } = parseNodeArgs(rawArgs); - expect(nodeArgs).toEqual(expectedNodeArgs); - expect(cliArgs).toEqual(expectedCliArgs); - }); - }); - - it('is able to pass the options flags to node js', (done) => { - const { stdout } = run( - __dirname, - [ - '--node-args', - `--require=${resolve(__dirname, 'bootstrap.js')}`, - '--node-args', - `-r ${resolve(__dirname, 'bootstrap2.js')}`, - '--output', - './bin/[name].bundle.js', - ], - false, - ); + it('is able to pass the options flags to node js', async (done) => { + const { stdout, stderr } = await run(__dirname, ['--output', './bin/[name].bundle.js'], false, [ + `--require=${resolve(__dirname, 'bootstrap.js')}`, + `--require=${resolve(__dirname, 'bootstrap2.js')}`, + ]); expect(stdout).toContain('---from bootstrap.js---'); expect(stdout).toContain('---from bootstrap2.js---'); + expect(stderr).toBeFalsy(); stat(resolve(__dirname, './bin/main.bundle.js'), (err, stats) => { expect(err).toBe(null); expect(stats.isFile()).toBe(true); @@ -60,20 +22,20 @@ describe('node flags', () => { }); }); - it('throws an error on supplying unknown flags', () => { - const { stderr } = run(__dirname, ['--node-args', '--unknown']); + it('throws an error on supplying unknown flags', async () => { + const { stderr } = await run(__dirname, [], false, ['--unknown']); expect(stderr).toContain('bad option'); }); - it('throws an error if no values were supplied with --max-old-space-size', () => { - const { stderr, stdout } = run(__dirname, ['--node-args', '--max-old-space-size']); + it('throws an error if no values were supplied with --max-old-space-size', async () => { + const { stderr, stdout } = await run(__dirname, [], false, ['--max-old-space-size']); expect(stderr).toContain('missing value for flag --max-old-space-size'); expect(stdout).toBeFalsy(); }); - it('throws an error if an illegal value was supplied with --max-old-space-size', () => { - const { stderr, stdout } = run(__dirname, ['--node-args', '--max-old-space-size=1024a']); - expect(stderr).toContain('illegal value for flag --max-old-space-size'); + it('throws an error if an illegal value was supplied with --max-old-space-size', async () => { + const { stderr, stdout } = await run(__dirname, [], true, ['--max_old_space_size=1024a']); + expect(stderr).toContain('Error: illegal value for flag --max_old_space_size=1024a of type size_t'); expect(stdout).toBeFalsy(); }); }); diff --git a/test/utils/test-utils.js b/test/utils/test-utils.js index 80faa787a27..8d96d872333 100644 --- a/test/utils/test-utils.js +++ b/test/utils/test-utils.js @@ -2,7 +2,7 @@ const path = require('path'); const fs = require('fs'); const execa = require('execa'); -const { sync: spawnSync } = execa; +const { sync: spawnSync, node: execaNode } = execa; const { Writable } = require('readable-stream'); const concat = require('concat-stream'); @@ -15,16 +15,18 @@ const ENABLE_LOG_COMPILATION = process.env.ENABLE_PIPE || false; * @param {String} testCase The path to folder that contains the webpack.config.js * @param {Array} args Array of arguments to pass to webpack * @param {Boolean} setOutput Boolean that decides if a default output path will be set or not - * @returns {Object} The webpack output + * @returns {Object} The webpack output or Promise when nodeOptions are present */ -function run(testCase, args = [], setOutput = true) { +function run(testCase, args = [], setOutput = true, nodeArgs = []) { const cwd = path.resolve(testCase); const outputPath = path.resolve(testCase, 'bin'); + const processExecutor = nodeArgs.length ? execaNode : spawnSync; const argsWithOutput = setOutput ? args.concat('--output', outputPath) : args; - const result = spawnSync(WEBPACK_PATH, argsWithOutput, { + const result = processExecutor(WEBPACK_PATH, argsWithOutput, { cwd, reject: false, + nodeOptions: nodeArgs, stdio: ENABLE_LOG_COMPILATION ? 'inherit' : 'pipe', });