-
Notifications
You must be signed in to change notification settings - Fork 4k
fix(exec): npx to run specified version if multiple version exist globally #7587
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
Conversation
7f945d2 to
2a9292c
Compare
2a9292c to
95c6997
Compare
95c6997 to
afe4815
Compare
workspaces/libnpmexec/lib/index.js
Outdated
| const nodesBySpec = tree.inventory.query('packageName', spec.name) | ||
| for (const node of nodesBySpec) { | ||
| // continue if node is not at specified depth, no need to check further | ||
| if (depth !== undefined && node.depth !== depth) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great solution. However we are currently not really using depth so much as telling this function to only look at depth 0.
It would probably make more sense in the future when reading this if this was called something like shallow and was a Boolean. and then if (shallow && node.depth) would be the test for continuing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.. Corrected in latest push.
f891839 to
f966233
Compare
workspaces/libnpmexec/lib/index.js
Outdated
| const { manifest: globalManifest } = | ||
| await missingFromTree({ spec, tree: globalTree, flatOptions }) | ||
| if (!globalManifest && await fileExists(`${globalBin}/${args[0]}`)) { | ||
| const { node: globalNode } = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we switch from manifest to node here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially I kept both, but the missingFromTree function in all code path it returns either { node } or { manifest } not sure if there ever a case if manifest is null and node is null as well, and also later we were checking for !manifest so changed to node, but I think now that you pointed it out it wont make a difference even if I keep it as it was earlier.
f966233 to
72125a8
Compare
When multiple version of the same package is exist globally either at top level or at any level as a sub dependency, even though the version specified does not exist at top level it was running top level bin since it matches the bin name.
This fixes checks for depth of the found node along with already existing specs checks.
Fixes: #7486