raineorshine / npm-check-updates

Find newer versions of package dependencies than what your package.json allows
Other
9.4k stars 327 forks source link

cli options don't override the default options in .ncurc.js #1355

Open its-dibo opened 11 months ago

its-dibo commented 11 months ago

when you set default options in .ncurc.js, these options don't be overridden by those that provided via cli

for example if you have a .ncurc.js like this

module.exports = {
  target: "minor"
  ...
}

then you triggered the following command

ncu --target=latest

then, the minor target is used

raineorshine commented 11 months ago

The syntax is actually ncu --target latest.

its-dibo commented 11 months ago

nice. it should also support this syntax because it is widly used and more readable. typically tools like commander, yargs and other cli parsers can read and parse this syntax

raineorshine commented 11 months ago

It uses commander, so I'm not sure why the equals delimiter is not working actually. I will look into that.

An option and its option-argument can be separated by a space, or combined into the same argument. The option-argument can follow the short option directly or follow an = for a long option.

serve -p 80
serve -p80
serve --port 80
serve --port=80
raineorshine commented 11 months ago

Okay, so that wasn't the problem at all. I'm just not use to the = delimiter, but it works as expected with npm-check-updates.

I set up the scenario you described, and am unable to reproduce the issue. The CLI option correctly overrides the .ncurc.js option:

raine[1355]% cat package.json                                                                                             ✓
{
  "dependencies": {
    "ncu-test-v2": "^1.0.0",
    "ncu-test-tag": "^1.0.0"
  }
}

raine[1355]% cat .ncurc.js                                                                                                ✓
module.exports = {
  target: "minor"
}

raine[1355]% ncu
Using config file /Users/raine/projects/ncu-issues/1355/.ncurc.js
Checking /Users/raine/projects/ncu-issues/1355/package.json
[====================] 2/2 100%

 ncu-test-tag  ^1.0.0  →  ^1.1.0

Run ncu -u to upgrade package.json

raine[1355]% ncu --target=latest                                                                                          ✓
Using config file /Users/raine/projects/ncu-issues/1355/.ncurc.js
Checking /Users/raine/projects/ncu-issues/1355/package.json
[====================] 2/2 100%

 ncu-test-tag  ^1.0.0  →  ^1.1.0
 ncu-test-v2   ^1.0.0  →  ^2.0.0

Run ncu --target=latest -u to upgrade package.json

If you are not seeing the expected upgrades, it is probably due to something else. Feel free to post a package.json and .ncurc.js that reproduce the issue and I'd be happy to investigate further.

its-dibo commented 11 months ago

ok, let me reproduce the issue again and give you a feedback

raineorshine commented 7 months ago

Let me know if you can reproduce the issue, otherwise I'll close this for now.

jftanner commented 1 month ago

I've got this too. There are a few packages whose next major version I know I can't accept, so I've got a custom target function to switch those to minor instead. (I don't want to filter them, because I still want any backported security fixes.)

My .ncurc.js file:

/**
 * A List of packages that cannot be updated to their next major version.
 */
const MAJOR_BLOCKED = [
  // Removed for security-through-obscurity reasons.
];

module.exports = {
  interactive: true, // Run interactively.
  format: ['group'], // Group by upgrade type (patch, minor, major, etc.).
  deep: true, // Include the client as well.

  // Only accept minor updates for any of the blocked packages.
  target: (packageName) => {
    const isBlocked = MAJOR_BLOCKED.includes(packageName.replace(/^@types\//, ''));
    return isBlocked ? 'minor' : 'latest';
  }
};

That works great, and does what I want normally. But if I use npx npm-check-updates --target minor then I still get major releases instead of minor ones.

raineorshine commented 1 month ago

Thanks, I was able to reproduce the issue with your example.

The problem is that in deep mode the config options for some reason override the cli arguments:

https://github.com/raineorshine/npm-check-updates/blob/27156434c2936568b2d3be929c9eaa4c084ecc8d/src/index.ts#L214-L219

This appears to have been added in #871.

Unfortunately when I switched the order of precedence, one of the deep tests broke, so someone will have to investigate more to provide a proper fix. Either the deep test is incorrect, the deep behavior is incorrect, or the cli arguments need to be differentiated from the config options in deep mode to ensure they are always applied.

jftanner commented 1 month ago

I owe you a PR for #1452, so I can take a look at this while I'm in there. (They're not directly related, but it's all config parsing.)

BTW, I worked around it with a pair of aliases and a tweak to my target function.

Aliases:

# Use `ncu` to run the latest npm-check-updates version interactively, whether or not an .ncurc.js file is present.
alias ncu='npx npm-check-updates -i'

# Use `ncu-minor` to target minor, even if a (compatible) .ncurc.js file is overriding the target.
alias ncu-minor='TARGET_VERSION=minor npx npm-check-updates -i --target minor'

And a generic .ncurc.js example that should work:

module.exports = {
  interactive: true,
  format: ['group'],
  deep: true,
  target: (packageName) => {
    // Use custom logic to override whatever packages are needed.
    const overrideTarget = null; // TODO Add custom logic.

    // Use the TARGET_VERSION env for the default, or `minor` if unset.
    // Workaround for https://github.com/raineorshine/npm-check-updates/issues/1355
    const defaultTarget = process.env.TARGET_VERSION || 'minor';

    // Use the override if set, otherwise the default.
    return overrideTarget || defaultTarget
  }
};
jftanner commented 1 month ago

Much like #1452, this only occurs with --deep. In that mode, it re-loads the .ncurc file(s) and applies the fetched config with a higher priority than the already-loaded options.

At the moment, the only difference between mergeConfig on or off is whether or not it merges arrays. In either case, the resulting pkgOptions is {...options, ...rcConfig}.

The problem is that options is already a mix of command-line and higher-level options. The loaded rcConfig is in the middle of that hierarchy, so it's wrong either way you put it. To fix it, you'd need to store the command-line options separately so they can be applied over the top.