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

[BUG] --scripts-prepend-node-path doesn't populate PATH #2808

Closed
Gerrit-K opened this issue Mar 2, 2021 · 12 comments · Fixed by #3353
Closed

[BUG] --scripts-prepend-node-path doesn't populate PATH #2808

Gerrit-K opened this issue Mar 2, 2021 · 12 comments · Fixed by #3353
Assignees
Labels
Bug thing that needs fixing pr: needs documentation pull request requires docs before merging Priority 1 high priority issue Release 7.x work is associated with a specific npm 7 release

Comments

@Gerrit-K
Copy link

Gerrit-K commented Mar 2, 2021

Current Behavior:

The flag --scripts-prepend-node-path, which is supposed to add the calling node executable to the PATH variable, doesn't seem to work. The directory isn't added.

Expected Behavior:

The directory of the calling node executable should be added to PATH.

Steps To Reproduce:

  • Download NodeJS and npm via nvm (in my case 14.15.5 and 7.6.0 respectively)
  • Create some skeleton with a package.json (in my case npm init nx-workspace)
  • Call node and npm in isolation to any shell environment:
    env -i bash --noprofile --norc -c '/path/to/.nvm/versions/node/v14.15.5/bin/node /path/to/.nvm/versions/node/v14.15.5/lib/node_modules/npm/bin/npm-cli.js --scripts-prepend-node-path run nx'
    (note: in order to fix sh: nx: command not found I've prepended the node call with PATH=$PATH:/path/to/.nvm/versions/node/v14.15.5/lib/node_modules/nx/bin)

Expected Behavior:

The help message of nx

Current Behavior:

env: node: No such file or directory

If you add a script with env or echo $PATH to the package.json you can see that PATH doesn't contain the directory to the calling node executable, so I assume it's not a bug of nx.

Environment:

  • macOS 11.1
  • NodeJS 14.15.5
  • npm 7.6.0
@Gerrit-K Gerrit-K added Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release labels Mar 2, 2021
@ljharb
Copy link
Contributor

ljharb commented Mar 2, 2021

I’m confused, how would npm run if node wasn’t in the PATH already?

@Gerrit-K
Copy link
Author

Gerrit-K commented Mar 2, 2021

As commented by calling npm-cli.js with a node binary, both referenced with the full path

@ljharb
Copy link
Contributor

ljharb commented Mar 2, 2021

ok - why would you do that tho, if you already have the full path?

@Gerrit-K
Copy link
Author

Gerrit-K commented Mar 2, 2021

I'm not actively doing that. It's the way IDEs like WebStorm call scripts with a node and npm version that you can select in the preferences or on a per-project basis. For reference, see this issue (and feel free to comment there, too).

@ljharb
Copy link
Contributor

ljharb commented Mar 2, 2021

Running npm or node without node and npm in the PATH is a bad idea, because many things will expect it to be there - especially npm run-scripts. The comment that "that's what npm does" isn't really relevant, because WebStorm isn't npm.

@Gerrit-K
Copy link
Author

Gerrit-K commented Mar 2, 2021

Well, I agree that it's probably not the intended way to run it and also that npm is not responsible for any failing sub script, but the docs clearly define how --scripts-prepend-node-path should work:

npm run sets the NODE environment variable to the node executable with which npm is executed. Also, if the --scripts-prepend-node-path is passed, the directory within which node resides is added to the PATH. If --scripts-prepend-node-path=auto is passed (which has been the default in npm v3), this is only performed when that node executable is not found in the PATH.

And as far as I can see it, that's not working in my (maybe exotic, but still valid) case. (edit: of course that's assuming that npm-cli.js should be able to be run via an arbitrary node executable)

Another edit:

The comment that "that's what npm does" isn't really relevant, because WebStorm isn't npm.

I believe the JetBrains employee was referencing this line from the npm shell wrapper.

@ljharb
Copy link
Contributor

ljharb commented Mar 3, 2021

right - that’s not something anything else should be doing. npm is part of the platform, webstorm is not.

@Gerrit-K
Copy link
Author

Gerrit-K commented Mar 3, 2021

Okay, let's put WebStorm aside for a moment and look at the simple question: "Is calling npm-cli.js with an arbitrary node version supported or not?".

If not and if you could point me to some sort of documentation about this, I'd gladly pass that information on to JetBrains and ask them to change how they call npm.

@Gerrit-K
Copy link
Author

Gerrit-K commented Mar 3, 2021

I just noticed npm/statusboard#117. If I interpret that correctly, the flag in question isn't even in the code anymore (which would explain why I wasn't able to find it after all). Can you confirm that?

@darcyclarke
Copy link
Contributor

@Gerrit-K apologize for the confusion here; --scripts-prepend-node-path was actually removed for npm v7 & that doc you referenced is out-of-date (will leave this open until we fix that reference). If you'd like us to reintroduce this flag, can you create an RRFC of RFC over at https://github.com/npm/rfcs

@darcyclarke darcyclarke added pr: needs documentation pull request requires docs before merging Priority 1 high priority issue and removed Needs Triage needs review for next steps labels Apr 16, 2021
@darcyclarke darcyclarke added this to the OSS - Sprint 28 milestone Apr 16, 2021
@Gerrit-K
Copy link
Author

@darcyclarke thanks, that clarification helps a lot. I'll point JetBrains to this issue and let them know that they need to adjust their calling code.

@Fryuni
Copy link

Fryuni commented Jun 7, 2021

Quite odd, an RFC is required to put the flag back but there is no RFC or even a mention on a commit message anywhere in that repository about removing it. It was just gone breaking every development environment that uses more than one node version.

It is also not mentioned in any changelog here since 2016 when it was added. The problem here is clearly not about docs, that option should not have been removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing pr: needs documentation pull request requires docs before merging Priority 1 high priority issue Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants