From 45af6562c22652404993fb1d77616fadf3e5eff2 Mon Sep 17 00:00:00 2001 From: Simon Vocella Date: Mon, 10 Apr 2017 18:43:24 +0200 Subject: [PATCH] Reduce complexity of src/cli/index.js (#2887) * clarify initial part of src/cli/index.js when we extracting arguments; flags and arguments after -- * change test if we do not provide any command name and specify a flag because the previous one was flaky * fix silent error in test when we expect an error from a yarn command * enrich commands with aliases * simplify if statement: if no command we set install as default * simplify if statement: we always have command with value undefined * put every logic related to help in help command * use deconstructuring instead of concat and some shift/unshit command * remove useless invariant on commandName: we are sure that is always defined * if command is not recognized set default to run; if command is run we set commandName as the first arg for npm_config_argv * add some tests cases when we do not recognize command * we use commander.js only to parse flags, remove every logic to put commandName and place only a placeholder * implement hasWrapper function for help command * display correct help link for aliases * display documentation link correctly in every case * add some console.asserts to ensure that we early crash if something is not expected * fix erronous case: yarn constructor --- __tests__/index.js | 79 ++++++++++++++++--- __tests__/lifecycle-scripts.js | 3 + src/cli/commands/help.js | 57 ++++++++++---- src/cli/index.js | 134 ++++++++++++--------------------- src/rc.js | 2 +- 5 files changed, 163 insertions(+), 112 deletions(-) diff --git a/__tests__/index.js b/__tests__/index.js index 3c1a2eb370..a4d2d7aa57 100644 --- a/__tests__/index.js +++ b/__tests__/index.js @@ -26,9 +26,9 @@ Promise> { } return new Promise((resolve, reject) => { - exec(`node "${yarnBin}" ${cmd} ${args.join(' ')}`, {cwd:workingDir, env:process.env}, (err, stdout) => { - if (err) { - reject(err); + exec(`node "${yarnBin}" ${cmd} ${args.join(' ')}`, {cwd:workingDir, env:process.env}, (error, stdout) => { + if (error) { + reject({error, stdout}); } else { const stdoutLines = stdout.toString() .split('\n') @@ -76,13 +76,23 @@ function expectHelpOutputAsSubcommand(stdout) { } function expectAnErrorMessage(command: Promise>, error: string) : Promise { - return command.catch((reason) => - expect(reason.message).toContain(error), + return command + .then(function() { + throw new Error('the command did not fail'); + }) + .catch((reason) => + expect(reason.error.message).toContain(error), ); } -function expectInstallOutput(stdout) { - expect(stdout[0]).toEqual(`yarn install v${pkg.version}`); +function expectAnInfoMessageAfterError(command: Promise>, info: string) : Promise { + return command + .then(function() { + throw new Error('the command did not fail'); + }) + .catch((reason) => + expect(reason.stdout).toContain(info), + ); } test.concurrent('should add package', async () => { @@ -182,12 +192,12 @@ test.concurrent('should run --version command', async () => { test.concurrent('should install if no args', async () => { const stdout = await execCommand('', [], 'run-add', true); - expectInstallOutput(stdout); + expect(stdout[0]).toEqual(`yarn install v${pkg.version}`); }); test.concurrent('should install if first arg looks like a flag', async () => { - const stdout = await execCommand('--offline', [], 'run-add', true); - expectInstallOutput(stdout); + const stdout = await execCommand('--json', [], 'run-add', true); + expect(stdout[stdout.length - 1]).toEqual('{"type":"success","data":"Saved lockfile."}'); }); test.concurrent('should interpolate aliases', async () => { @@ -197,6 +207,13 @@ test.concurrent('should interpolate aliases', async () => { ); }); +test.concurrent('should display correct documentation link for aliases', async () => { + await expectAnInfoMessageAfterError( + execCommand('i', [], 'run-add', true), + 'Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.', + ); +}); + test.concurrent('should run help of run command if --help is before --', async () => { const stdout = await execCommand('run', ['custom-script', '--help', '--'], 'run-custom-script-with-arguments'); expect(stdout[0]).toEqual('Usage: yarn [command] [flags]'); @@ -216,3 +233,45 @@ test.concurrent('should run bin command', async () => { expect(stdout[0]).toEqual(path.join(fixturesLoc, 'node_modules', '.bin')); expect(stdout.length).toEqual(1); }); + +test.concurrent('should throws missing command for not camelised command', async () => { + await expectAnErrorMessage( + execCommand('HelP', [], 'run-add', true), + 'Command \"HelP\" not found', + ); +}); + +test.concurrent('should throws missing command for not alphabetic command', async () => { + await expectAnErrorMessage( + execCommand('123', [], 'run-add', true), + 'Command \"123\" not found', + ); +}); + +test.concurrent('should throws missing command for unknown command', async () => { + await expectAnErrorMessage( + execCommand('unknown', [], 'run-add', true), + 'Command \"unknown\" not found', + ); +}); + +test.concurrent('should not display documentation link for unknown command', async () => { + await expectAnInfoMessageAfterError( + execCommand('unknown', [], 'run-add', true), + '', + ); +}); + +test.concurrent('should display documentation link for known command', async () => { + await expectAnInfoMessageAfterError( + execCommand('add', [], 'run-add', true), + 'Visit https://yarnpkg.com/en/docs/cli/add for documentation about this command.', + ); +}); + +test.concurrent('should throws missing command for constructor command', async () => { + await expectAnErrorMessage( + execCommand('constructor', [], 'run-add', true), + 'Command \"constructor\" not found', + ); +}); diff --git a/__tests__/lifecycle-scripts.js b/__tests__/lifecycle-scripts.js index 51e77633f1..d7e900b903 100644 --- a/__tests__/lifecycle-scripts.js +++ b/__tests__/lifecycle-scripts.js @@ -67,6 +67,9 @@ async () => { stdout = await execCommand('', 'npm_config_argv_env_vars', env); expect(stdout).toContain('##install##'); + stdout = await execCommand('run test', 'npm_config_argv_env_vars', env); + expect(stdout).toContain('##test##'); + stdout = await execCommand('test', 'npm_config_argv_env_vars', env); expect(stdout).toContain('##test##'); }); diff --git a/src/cli/commands/help.js b/src/cli/commands/help.js index c2398cf712..a8cb225698 100644 --- a/src/cli/commands/help.js +++ b/src/cli/commands/help.js @@ -4,9 +4,13 @@ import * as commands from './index.js'; import * as constants from '../../constants.js'; import type {Reporter} from '../../reporters/index.js'; import type Config from '../../config.js'; -import {sortAlpha, hyphenate} from '../../util/misc.js'; +import {sortAlpha, hyphenate, camelCase} from '../../util/misc.js'; const chalk = require('chalk'); +export function hasWrapper(): boolean { + return false; +} + export function run( config: Config, reporter: Reporter, @@ -17,24 +21,47 @@ export function run( const getDocsInfo = (name) => 'Visit ' + chalk.bold(getDocsLink(name)) + ' for documentation about this command.'; if (args.length) { - const helpCommand = hyphenate(args[0]); - if (commands[helpCommand]) { - commander.on('--help', () => console.log(' ' + getDocsInfo(helpCommand) + '\n')); - } - } else { - commander.on('--help', () => { - console.log(' Commands:\n'); - for (const name of Object.keys(commands).sort(sortAlpha)) { - if (commands[name].useless) { - continue; + const commandName = camelCase(args.shift()); + + if (commandName) { + const command = commands[commandName]; + + if (command) { + if (typeof command.setFlags === 'function') { + command.setFlags(commander); + } + + const examples: Array = (command && command.examples) || []; + if (examples.length) { + commander.on('--help', () => { + console.log(' Examples:\n'); + for (const example of examples) { + console.log(` $ yarn ${example}`); + } + console.log(); + }); } + commander.on('--help', () => console.log(' ' + getDocsInfo(commandName) + '\n')); - console.log(` - ${hyphenate(name)}`); + commander.help(); + return Promise.resolve(); } - console.log('\n Run `' + chalk.bold('yarn help COMMAND') + '` for more information on specific commands.'); - console.log(' Visit ' + chalk.bold(getDocsLink()) + ' to learn more about Yarn.\n'); - }); + } } + + commander.on('--help', () => { + console.log(' Commands:\n'); + for (const name of Object.keys(commands).sort(sortAlpha)) { + if (commands[name].useless) { + continue; + } + + console.log(` - ${hyphenate(name)}`); + } + console.log('\n Run `' + chalk.bold('yarn help COMMAND') + '` for more information on specific commands.'); + console.log(' Visit ' + chalk.bold(getDocsLink()) + ' to learn more about Yarn.\n'); + }); + commander.help(); return Promise.resolve(); } diff --git a/src/cli/index.js b/src/cli/index.js index cf550e95dd..5909b9f0c7 100644 --- a/src/cli/index.js +++ b/src/cli/index.js @@ -2,7 +2,7 @@ import {ConsoleReporter, JSONReporter} from '../reporters/index.js'; import {registries, registryNames} from '../registries/index.js'; -import * as commands from './commands/index.js'; +import * as _commands from './commands/index.js'; import * as constants from '../constants.js'; import * as network from '../util/network.js'; import {MessageError} from '../errors.js'; @@ -24,25 +24,21 @@ const pkg = require('../../package.json'); loudRejection(); -// +const commands = {..._commands}; +for (const key in aliases) { + commands[key] = { + run(config: Config, reporter: ConsoleReporter | JSONReporter): Promise { + throw new MessageError(`Did you mean \`yarn ${aliases[key]}\`?`); + }, + }; +} + const startArgs = process.argv.slice(0, 2); -let args = process.argv.slice(2); // ignore all arguments after a -- -let endArgs = []; -for (let i = 0; i < args.length; i++) { - const arg = args[i]; - if (arg === '--') { - endArgs = args.slice(i + 1); - args = args.slice(0, i); - } -} - -// NOTE: Pending resolution of https://github.com/tj/commander.js/issues/346 -// Remove this (and subsequent use in the logic below) after bug is resolved and issue is closed -const ARGS_THAT_SHARE_NAMES_WITH_OPTIONS = [ - 'version', -]; +const doubleDashIndex = process.argv.findIndex((element) => element === '--'); +const args = process.argv.slice(2, doubleDashIndex === -1 ? process.argv.length : doubleDashIndex); +const endArgs = doubleDashIndex === -1 ? [] : process.argv.slice(doubleDashIndex + 1, process.argv.length); // set global options commander.version(pkg.version); @@ -98,90 +94,57 @@ commander.option('--network-concurrency ', 'maximum number of concurrent commander.option('--network-timeout ', 'TCP timeout for network requests', parseInt); commander.option('--non-interactive', 'do not show interactive prompts'); -// get command name -let commandName: ?string = args.shift() || ''; -let command; - -// const getDocsLink = (name) => `${constants.YARN_DOCS}${name || ''}`; const getDocsInfo = (name) => 'Visit ' + chalk.bold(getDocsLink(name)) + ' for documentation about this command.'; -// +// get command name +let commandName: string = args.shift() || 'install'; + if (commandName === '--help' || commandName === '-h') { commandName = 'help'; } -// if no args or command name looks like a flag then default to `install` -if (!commandName || commandName[0] === '-') { - if (commandName) { - args.unshift(commandName); - } - commandName = 'install'; +if (args.indexOf('--help') >= 0 || args.indexOf('-h') >= 0) { + args.unshift(commandName); + commandName = 'help'; } -// aliases: i -> install -if (commandName && typeof aliases[commandName] === 'string') { - const alias = aliases[commandName]; - command = { - run(config: Config, reporter: ConsoleReporter | JSONReporter): Promise { - throw new MessageError(`Did you mean \`yarn ${alias}\`?`); - }, - }; +// if no args or command name looks like a flag then set default to `install` +if (commandName[0] === '-') { + args.unshift(commandName); + commandName = 'install'; } -// -if (commandName === 'help' && args.length) { - commandName = camelCase(args.shift()); - args.push('--help'); +let command; +const camelised = camelCase(commandName); +if (camelised && Object.prototype.hasOwnProperty.call(commands, camelised)) { + command = commands[camelised]; } -// -invariant(commandName, 'Missing command name'); +// if command is not recognized, then set default to `run` if (!command) { - const camelised = camelCase(commandName); - if (camelised) { - command = commands[camelised]; - } + args.unshift(commandName); + command = commands.run; } -// if (command && typeof command.setFlags === 'function') { command.setFlags(commander); } -if (args.indexOf('--help') >= 0 || args.indexOf('-h') >= 0) { - const examples: Array = (command && command.examples) || []; - if (examples.length) { - commander.on('--help', () => { - console.log(' Examples:\n'); - for (const example of examples) { - console.log(` $ yarn ${example}`); - } - console.log(); - }); - } - commander.on('--help', () => console.log(' ' + getDocsInfo(commandName) + '\n')); - - commander.parse(startArgs.concat(args)); - commander.help(); - process.exit(1); -} - -args = [commandName].concat(getRcArgs(commandName), args); - -if (ARGS_THAT_SHARE_NAMES_WITH_OPTIONS.indexOf(commandName) >= 0 && args[0] === commandName) { - args.shift(); -} - -commander.parse(startArgs.concat(args)); +commander.parse([ + ...startArgs, + // we use this for https://github.com/tj/commander.js/issues/346, otherwise + // it will strip some args that match with any options + 'this-arg-will-get-stripped-later', + ...getRcArgs(commandName), + ...args, +]); commander.args = commander.args.concat(endArgs); -if (command) { - commander.args.shift(); -} else { - command = commands.run; -} -invariant(command, 'missing command'); +// we strip cmd +console.assert(commander.args.length >= 1); +console.assert(commander.args[0] === 'this-arg-will-get-stripped-later'); +commander.args.shift(); // let Reporter = ConsoleReporter; @@ -207,7 +170,7 @@ if (typeof command.hasWrapper === 'function') { if (commander.json) { outputWrapper = false; } -if (outputWrapper && commandName !== 'help') { +if (outputWrapper) { reporter.header(commandName, pkg); } @@ -382,7 +345,7 @@ config.init({ httpsProxy: commander.httpsProxy, networkConcurrency: commander.networkConcurrency, nonInteractive: commander.nonInteractive, - commandName, + commandName: commandName === 'run' ? commander.args[0] : commandName, }).then(() => { // option "no-progress" stored in yarn config @@ -423,11 +386,10 @@ config.init({ onUnexpectedError(err); } - if (commandName) { - const actualCommandForHelp = commands[commandName] ? commandName : aliases[commandName]; - if (command && actualCommandForHelp) { - reporter.info(getDocsInfo(actualCommandForHelp)); - } + if (aliases[commandName]) { + reporter.info(getDocsInfo(aliases[commandName])); + } else if (commands[commandName]) { + reporter.info(getDocsInfo(commandName)); } process.exit(1); diff --git a/src/rc.js b/src/rc.js index 29ae25f4b3..7d22abaff7 100644 --- a/src/rc.js +++ b/src/rc.js @@ -70,7 +70,7 @@ export function getRcArgs(command: string): Array { let result = rcArgsCache['*'] || []; - if (command !== '*') { + if (command !== '*' && Object.prototype.hasOwnProperty.call(rcArgsCache, command)) { result = result.concat(rcArgsCache[command] || []); }