Skip to content

Commit

Permalink
fix(cli): remove usage of deprecated npm env var from arg parser (#3486)
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` v7+, 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 authored Jul 22, 2022
1 parent bbdebf4 commit 22d9858
Show file tree
Hide file tree
Showing 5 changed files with 169 additions and 539 deletions.
34 changes: 4 additions & 30 deletions src/cli/parse-flags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,12 @@ 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
* @param _sys an optional compiler system
* @returns a structured ConfigFlags object
*/
export const parseFlags = (args: string[], sys?: CompilerSystem): ConfigFlags => {
export const parseFlags = (args: string[], _sys?: CompilerSystem): ConfigFlags => {
// TODO(STENCIL-509): remove the _sys parameter here ^^ (for v3)
const flags: ConfigFlags = createConfigFlags();

// cmd line has more priority over npm scripts cmd
Expand All @@ -33,17 +34,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 +334,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;
};
3 changes: 2 additions & 1 deletion src/cli/public.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export declare function run(init: CliInitOptions): Promise<void>;
*/
export declare function runTask(coreCompiler: any, config: Config, task: TaskCommand): Promise<void>;

export declare function parseFlags(args: string[], sys?: CompilerSystem): ConfigFlags;
// TODO(STENCIL-509): remove the _sys parameter here (for v3)
export declare function parseFlags(args: string[], _sys?: CompilerSystem): ConfigFlags;

export { CompilerSystem, Config, ConfigFlags, Logger, TaskCommand };
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 22d9858

Please sign in to comment.