Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: do not spawn new process for running webpack #1741

Merged
merged 9 commits into from
Aug 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/webpack-cli/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
```
Expand Down
2 changes: 1 addition & 1 deletion packages/webpack-cli/__tests__/cli-executer.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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']]);

Expand Down
6 changes: 2 additions & 4 deletions packages/webpack-cli/bin/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand All @@ -13,6 +12,5 @@ if (importLocal(__filename)) {
process.title = 'webpack';

const [, , ...rawArgs] = process.argv;
const { cliArgs, nodeArgs } = parseNodeArgs(rawArgs);

runner(nodeArgs, cliArgs);
runCLI(rawArgs);
31 changes: 17 additions & 14 deletions packages/webpack-cli/lib/bootstrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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');
Expand All @@ -104,6 +109,4 @@ async function runCLI(cli, commandIsUsed) {
}
}

const commandIsUsed = isCommandUsed(commands);
const cli = new WebpackCLI();
runCLI(cli, commandIsUsed);
module.exports = runCLI;
4 changes: 2 additions & 2 deletions packages/webpack-cli/lib/utils/cli-executer.js
Original file line number Diff line number Diff line change
@@ -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 = [];
Expand Down Expand Up @@ -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.`);
}
Expand Down
8 changes: 0 additions & 8 deletions packages/webpack-cli/lib/utils/cli-flags.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 <value>',
Expand Down
27 changes: 0 additions & 27 deletions packages/webpack-cli/lib/utils/parse-node-args.js

This file was deleted.

4 changes: 2 additions & 2 deletions packages/webpack-cli/lib/webpack-cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
}
Expand Down
70 changes: 16 additions & 54 deletions test/node/node.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,78 +2,40 @@
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=<args> 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);
done();
});
});

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');
anshumanv marked this conversation as resolved.
Show resolved Hide resolved
expect(stdout).toBeFalsy();
});
});
10 changes: 6 additions & 4 deletions test/utils/test-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand All @@ -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',
});

Expand Down