fix: $npm_execpath always points to npm, even when running npx

[rotu]

When running a script with npx (but not with npm exec), $npm_execpath is set to ".../npm/bin/npx-cli.js". This causes packages to fail to detect the package manager.

Now $npm_execpath will always be the path to npm-cli.js.

References

Fixes #6662

[wraithgar]

Not sure why CI is having the specific errors it is, but when I run config tests locally they don't pass, and there are coverage issues. Does npm test -w @npmcli/config pass locally for you?

[rotu]

I'm surprised it's trying to run against node <18. This commit definitely broke the runs on node 14: b8a5764

No, the CI is not currently passing for me locally. I suspect it has to do with this gotcha from the docs:

> When the entry point is not a CommonJS module, require.main is undefined, and the main module is out of reach.

I had to adjust some tests to account for moving logic out of the flatten helper.

I'll figure out the coverage issue once I verify the tests run as expected.

[rotu]

The last commit (b8b1db3) I'm slightly unsure about.

This commit started writing to the variable: c3ba1da
But I can't figure out why we would ever read from this variable. It seems to maybe have originated in testing code here and accidentally copied into the main code flow: npm/config@fd6432e

EDIT: it also seems like inheriting this value is probably unexpected, given npm/config#12 for the closely related $INIT_CWD (which you may remember from the above-mentioned c3ba1da)

Your thoughts are much appreciated.

[rotu]

BTW, as of now, test cases all pass for me and coverage is at 100%.

[wraithgar]

I agree that the process.env.NODE is in all likelihood a dependency injection leftover from a test. I don't know if that means we can remove it.

Historically we have been removing the ability to change things like this, for instance in npm 10 re removed the ci-name config. Previously we also did not allow folks to change what the path to npm was in scripts.

However, removing this could be a breaking change to folks. We're gonna have to leave it in. However I also think we shouldn't worry about covering that in testing. Can you add /* istanbul ignore next */ to the line before it, and put it back in?

I'll add a note to the npm 11 roadmap to remove it for real.

[rotu]

However, removing this could be a breaking change to folks. We're gonna have to leave it in. However I also think we shouldn't worry about covering that in testing. Can you add /* istanbul ignore next */ to the line before it, and put it back in?

I thought about that and it makes me feel icky. The ignored path is the one MOST COMMON in practice. This PR is already a breaking change, and insofar as the NODE environment variable does anything, it’s a bug as well.

If you still insist, I can move it to another PR.

[wraithgar]

Ok lemme noodle on it.

Ultimately the NODE environment is in all likelihood trying to solve the problem of having everything in side the npm lifecycle use the same node version as the one that npm was ran with. I do remember this being an issue in the past, we don't want to support running different versions of node or npm in subprocesses. In the past this manifested as refusing to support a config that told npm what version of node to run, that's an OS concern.

I think removing it may be the right choice here but as you can imagine we are trying to be extremely cautious with things that could break other folks.

[rotu]

Ok lemme noodle on it.

@wraithgar, what did your noodle say?

[wraithgar]

We can re-export NODE if an edge case we didn't know about comes to light.

[rotu]

Woohoo!

We can re-export NODE if an edge case we didn't know about comes to light.

I didn’t stop exporting $NODE to subprocess - I stopped npm from importing it from the parent process.

[wraithgar]

Ok yeah that's what I get from typing from memory.