Skip to content

Commit

Permalink
fix(cli): remove usage of deprecated npm env var from arg parser
Browse files Browse the repository at this point in the history
This removes some code from our argument parsing implementation which
would look for an environment variable (`npm_config_argv`) set by npm
and, if present, merge CLI flags found their into the ones already
present on `process.argv`.

We want to remove this for two reasons:

- `npm_config_argv` is deprecated in `npm`, so in newer versions of the
  package manager it is no longer set and our code referencing it is
  effectively doing nothing
- `yarn` v1.x still sets it (presumably for compatibility with `npm`
  that hasn't been removed yet, which causes an inconsistency between
  `npm` and `yarn` where certain `package.json` scripts will run without
  issue in `npm` but fail in `yarn` (see #3482)

Accordingly, this commit deletes the offending function from
`src/cli/parse-flags.ts` and makes a few related changes at call sites,
etc which are necessary due to that change.

This commit also refactors the spec file for `parse-flags.ts` to remove
redundant tests. Because all of the supported CLI arguments (i.e. those
added to `knownArgs`) are defined in `ReadonlyArray<string>` variables
we can do a lot of exhaustive testing of all the arguments, set in the
various possible permutations, etc, so we don't need to define a bunch
of individual tests. Accordingly, the test file is refactored to remove
redundant tests, add a few more test cases for the exhaustive tests,
organize things a bit more with `describe` blocks, and ensure that we
have good tests around all off the 'edge-case-ey' things.
  • Loading branch information
alicewriteswrongs committed Jul 22, 2022
1 parent 47c4ccb commit 3910179
Show file tree
Hide file tree
Showing 4 changed files with 166 additions and 539 deletions.
34 changes: 3 additions & 31 deletions src/cli/parse-flags.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { CompilerSystem, LogLevel, LOG_LEVELS, TaskCommand } from '../declarations';
import { LogLevel, LOG_LEVELS, TaskCommand } from '../declarations';
import { dashToPascalCase, toDashCase } from '@utils';
import {
BOOLEAN_CLI_ARGS,
Expand All @@ -19,11 +19,10 @@ import {
/**
* Parse command line arguments into a structured `ConfigFlags` object
*
* @param args an array of config flags
* @param sys an optional compiler system
* @param args an array of CLI flags
* @returns a structured ConfigFlags object
*/
export const parseFlags = (args: string[], sys?: CompilerSystem): ConfigFlags => {
export const parseFlags = (args: string[]): ConfigFlags => {
const flags: ConfigFlags = createConfigFlags();

// cmd line has more priority over npm scripts cmd
Expand All @@ -33,17 +32,6 @@ export const parseFlags = (args: string[], sys?: CompilerSystem): ConfigFlags =>
}
parseArgs(flags, flags.args);

if (sys && sys.name === 'node') {
const envArgs = getNpmConfigEnvArgs(sys);
parseArgs(flags, envArgs);

envArgs.forEach((envArg) => {
if (!flags.args.includes(envArg)) {
flags.args.push(envArg);
}
});
}

if (flags.task != null) {
const i = flags.args.indexOf(flags.task);
if (i > -1) {
Expand Down Expand Up @@ -344,19 +332,3 @@ const isLogLevel = (maybeLogLevel: string): maybeLogLevel is LogLevel =>
//
// see microsoft/TypeScript#31018 for some discussion of this
LOG_LEVELS.includes(maybeLogLevel as any);

const getNpmConfigEnvArgs = (sys: CompilerSystem) => {
// process.env.npm_config_argv
// {"remain":["4444"],"cooked":["run","serve","--port","4444"],"original":["run","serve","--port","4444"]}
let args: string[] = [];
try {
const npmConfigArgs = sys.getEnvironmentVar('npm_config_argv');
if (npmConfigArgs) {
args = JSON.parse(npmConfigArgs).original as string[];
if (args[0] === 'run') {
args = args.slice(2);
}
}
} catch (e) {}
return args;
};
2 changes: 1 addition & 1 deletion src/cli/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export const run = async (init: d.CliInitOptions) => {
const { args, logger, sys } = init;

try {
const flags = parseFlags(args, sys);
const flags = parseFlags(args);
const task = flags.task;

if (flags.debug || flags.verbose) {
Expand Down
Loading

0 comments on commit 3910179

Please sign in to comment.