Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix --if-present cascading to child npm run scripts #3589

Closed
wants to merge 1 commit into from

Conversation

robin-drexler
Copy link

@robin-drexler robin-drexler commented Jul 28, 2021

This is an attempt to fix #3352

Given a package.json like:

{
  "scripts": {
    "present": "npm run  not-present"
  }
}

npm run --if-present present would result in exit code 0, even though the second npm run call doesn't specify the --if-present flag.

With this PR, this would now throw throw an error:

npm ERR! Missing script: "not-present"

If --if-present is specified in the second script, this will continue to successfully return.

{
  "scripts": {
    "present": "npm run  --if-present not-present"
  }
}

npm run --if-present present returns exit code 0.

A few notes:

I have no idea if the change might have an adverse effect on anything else or if it's the right approach at all. Feedback is welcome! :)
I was also wondering if similar changes need to be applied to other flags (like --silent), but probably not?

I have tried to write tests, but I'm a bit stuck because

const config = {
uses a mocked version of the config which doesn't distinguish between cli and other config.
Also I'm not sure the mocked npm could be used if a script uses npm run some-other-script inside that test?

@robin-drexler robin-drexler requested a review from a team as a code owner July 28, 2021 03:00
@robin-drexler robin-drexler changed the title fix --if-present cascading to chile npm run scripts fix --if-present cascading to child npm run scripts Jul 28, 2021
@isaacs
Copy link
Contributor

isaacs commented Jul 28, 2021

The correct way to do this is by putting envExport: false in the config definition for if-present.

As written, this PR would mean that npm_config_if_present=true npm run script-not-present (setting from the env) or npm config set if-present=true (setting in a config file) would not work properly.

It might be worth a conversation to see if not cascading if-present to child processes would cause problems in some cases. At the very least, you wouldn't be able to inspect the setting within a script, but I can't think of a reason why you might want to do that.

For context, we do not export the workspaces or workspace configs, because otherwise it would cause problems in a lot of cases. If a package had { "scripts": { "postinstall": "npm run build", "build": "dosomething" }} then installing the dependency in a workspace would trigger the npm run build with a workspace config, which is definitely not going to do anything sensible.

@isaacs
Copy link
Contributor

isaacs commented Jul 28, 2021

I guess the bigger question is: why don't you want it to persist? The notpresent script isn't present, but you did --if-present, so it seems surprising to me for it not to silently succeed in that case. But sometimes it's the case that I've been too close to npm for too long, so strange npm behaviors seem normal to me 😅

@robin-drexler
Copy link
Author

Thanks for getting back!

The correct way to do this is by putting envExport: false in the config definition for if-present.

TIL, I'll have a look at this.
Do I understand correctly that setting envExport: false would cause the cli flag not to be exported as npm config env, however config.get('if-present) (without setting the where param to cli) would still respect it if it is set by env or another "static" config?

Maybe that could be the perfect compromise between cascading and not cascading. Flag: doesn't cascade. static (for lack of a better term) config: does cascade.

As written, this PR would mean that npm_config_if_present=true npm run script-not-present (setting from the env) or npm config set if-present=true (setting in a config file) would not work properly.

Yeah, I thought about this too and wasn't sure if setting this as a config was even an "advertised" use case. I couldn't find anything in the docs about it. Which of course doesn't mean it's not there.

it seems surprising to me for it not to silently succeed in that case
To me it seems intuitive (but that's just me). Imagine you're running an optional build, so you go and run npm run --if-present build

You may not even know what the run command does exactly because you're a CI provider or whatever.
The author of the build script may never have expected its script to be run with --if-present.

Do you have any idea how this could be tested or if it needs testing?

@ljharb
Copy link
Contributor

ljharb commented Jul 28, 2021

It makes a lot of sense to me that if-present would not cascade - the point of doing npm run foo --if-present is that i explicitly want to avoid the error if foo is not present - it shouldn't have any effect on the implementation details of the "foo" script, and more importantly, when "foo" is present, it should have no effect at all.

@isaacs
Copy link
Contributor

isaacs commented Jul 28, 2021

It makes a lot of sense to me that if-present would not cascade - the point of doing npm run foo --if-present is that i explicitly want to avoid the error if foo is not present - it shouldn't have any effect on the implementation details of the "foo" script, and more importantly, when "foo" is present, it should have no effect at all.

Right, and if you did npm config set if-present=true, then that second command would just pick up the config file and be run with if-present, which is probably what you'd want.

Makes sense to me.

I wonder if there are other configs like this, where it doesn't make sense to cascade them through the environment?

@wraithgar
Copy link
Member

The mock config in the tests is something we know needs to be fixed, and there was a PR to fix it that ran into unrelated issues surrounding how our coverage works. Looks like I may need to re prioritize getting that PR over the finish line #3463

@darcyclarke darcyclarke added the Release 7.x work is associated with a specific npm 7 release label Aug 5, 2021
@darcyclarke darcyclarke added this to the OSS - Sprint 35 milestone Aug 9, 2021
@darcyclarke darcyclarke added Needs Discussion is pending a discussion Needs Review Enhancement new feature or improvement labels Aug 23, 2021
@wraithgar wraithgar added Release 8.x work is associated with a specific npm 8 release and removed Release 7.x work is associated with a specific npm 7 release labels Oct 7, 2021
@wraithgar wraithgar changed the base branch from latest to release-next January 26, 2022 20:44
@wraithgar wraithgar changed the base branch from release-next to latest March 9, 2022 18:25
ruyadorno added a commit to ruyadorno/cli that referenced this pull request Apr 4, 2022
Do not pass the `if-present` env config value to spawned processes.

Fixes: npm#3352
Close: npm#3589
wraithgar pushed a commit that referenced this pull request Apr 5, 2022
Do not pass the `if-present` env config value to spawned processes.

Fixes: #3352
Close: #3589
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement new feature or improvement Needs Discussion is pending a discussion Release 8.x work is associated with a specific npm 8 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] npm run <script name> --if-present silently ignores missing scripts in subcommands
5 participants