mysticatea / npm-run-all

A CLI tool to run multiple npm-scripts in parallel or sequential.
MIT License
5.72k stars 240 forks source link

Why is npm-run-all so narcissistic about the config field in package.json. #212

Open airtonix opened 3 years ago

airtonix commented 3 years ago

I mean, i love npm-run-all, but not everything in the world is about npm-run-all.

https://github.com/mysticatea/npm-run-all/blob/bf91f94ce597aa61da37d2e4208ce8c48bc86673/bin/common/parse-cli-args.js#L16 should be:

const PACKAGE_CONFIG_PATTERN = /^npm_package_config_npmrunall_(.+)$/

https://github.com/mysticatea/npm-run-all/blob/bf91f94ce597aa61da37d2e4208ce8c48bc86673/test-workspace/package.json#L9-L12

should be :

"config": {
  "npmrunall": {
    "DEST": "build",
    "test": "OK"
  },
},

https://github.com/mysticatea/npm-run-all/blob/bf91f94ce597aa61da37d2e4208ce8c48bc86673/test-workspace/tasks/package-config2.js#L4

should be :

appendResult(`${process.env.npm_package_config_npmrunall_test}\n${process.env.npm_package_config_npmrunall_test2}\n${process.env.npm_package_config_npmrunall_test3}`)

I mean, why are we reinventing solutions to the config/arg/env problem ? just use nconf:

import { join } from 'path';
import { userInfo } from 'os';
import { findRootSync } from "@manypkg/find-root";

import isRelative from 'is-relative';
import nconf from 'nconf';

const KEY = 'NPMRUNALL';
const RCFILENAME = `.${KEY.toLocaleLowerCase()}rc.json`;
const SEPARATOR = '__';
const PATTERN = new RegExp(`^${KEY}${SEPARATOR}`);

nconf.env({
  separator: SEPARATOR,
  match: PATTERN,
  lowerCase: true,
  parseValues: true,
  transform(obj: { key: string }) {
    obj.key.replace(PATTERN, '');
    return obj;
  },
});

nconf.argv({
  s: {
    alias: 'source',
    describe: 'Source directory',
    type: 'string',
    default: join(process.cwd(), 'out'),
    coerce: (value: string) =>
      isRelative(value) ? join(process.cwd(), value) : value,
  },
  t: {
    alias: 'target',
    describe: 'Target directory',
    type: 'string',
    default: join(process.cwd(), 'out'),
    coerce: (value: string) =>
      isRelative(value) ? join(process.cwd(), value) : value,
  },
  v: {
    alias: 'verbose',
    describe: 'expose a key in the release plan item to the command',
    type: 'boolean',
    default: false,
  },
});

nconf
  .file('user', {
    file: `${userInfo().homedir}/${RCFILENAME}`,
    dir: process.cwd(),
    search: true,
  })
  .file('cwd', {
    file: RCFILENAME,
    dir: process.cwd(),
    search: true,
  })
  .file('repo', {
    file: RCFILENAME,
    dir: findRootSync(process.cwd()),
    search: true,
  })
  .defaults({
    verbose: false,
  });

export const Options = nconf.get();

We shouldn't even be using the config field of the package.json.... it's an anti pattern.

airtonix commented 3 years ago

in addition the reason why npm-run-all has suddenly and recently become so naive about how the config field is used is due to #60 .

nconf solves that problem without conflating it with the config field in package.json.

in the end the major problem with this assumptive approach is that you're not namespacing the new config variables you create that originate in these ephemerally undocumented args.