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(libnpmexec): workspaces support #4643

Merged
merged 1 commit into from
Apr 19, 2022

Conversation

ruyadorno
Copy link
Contributor

@ruyadorno ruyadorno commented Mar 30, 2022

Fixes the proper path location to use when targetting specific workspaces.

References

Fixes: #3520
Relates to: npm/statusboard#403

@ruyadorno ruyadorno requested a review from a team as a code owner March 30, 2022 21:53
@ruyadorno ruyadorno added semver:patch semver patch level for changes Release 8.x work is associated with a specific npm 8 release release: next These items should be addressed in the next release labels Mar 30, 2022
@ruyadorno ruyadorno force-pushed the fix-npm-exec-for-workspaces branch 2 times, most recently from 0b405af to dd6530e Compare March 31, 2022 03:05
workspaces/libnpmexec/lib/index.js Outdated Show resolved Hide resolved
workspaces/libnpmexec/lib/index.js Outdated Show resolved Hide resolved
workspaces/libnpmexec/lib/index.js Outdated Show resolved Hide resolved
workspaces/libnpmexec/lib/index.js Outdated Show resolved Hide resolved
workspaces/libnpmexec/lib/index.js Outdated Show resolved Hide resolved
@ruyadorno
Copy link
Contributor Author

@nlf after jumping back at the code here I realized that I could get the original bugfix without explicit requiring that arborist-read-data-refactor 😊 I'm going to split these into two separate PRs so that they're more atomic, simpler to review and revert if needed be.

So here now is only the change required in lib/commands/exec.js in order to properly use localPrefix as the place to load arborist when running in the context of workspaces.

@ruyadorno ruyadorno marked this pull request as draft April 11, 2022 19:05
Fixes the proper path location to use when targetting specific
workspaces.

Fixes: npm#3520
Relates to: npm/statusboard#403
@ruyadorno ruyadorno force-pushed the fix-npm-exec-for-workspaces branch from 1175520 to 7c1faf5 Compare April 19, 2022 14:32
@ruyadorno ruyadorno marked this pull request as ready for review April 19, 2022 14:33
ruyadorno added a commit to ruyadorno/cli that referenced this pull request Apr 19, 2022
Refactor / clean up of the logic around reading installed
packages. Fixes reading packages from mixed sources (one being
from the local installed tree and the other from the registry
using pacote.manifest).

Makes it so that libnpmexec is always reading from the
Arborist actual tree instead of reading `node_modules` from
the file system when retrieving local package data.

Fixes: npm#3668
Relates to: npm#4643
Relates to: npm#4619
Relates to: npm/statusboard#403
Copy link
Contributor

@nlf nlf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix looks good, and these tests are great!

lukekarrys pushed a commit that referenced this pull request Apr 19, 2022
Refactor / clean up of the logic around reading installed
packages. Fixes reading packages from mixed sources (one being
from the local installed tree and the other from the registry
using pacote.manifest).

Makes it so that libnpmexec is always reading from the
Arborist actual tree instead of reading `node_modules` from
the file system when retrieving local package data.

Fixes: #3668
Relates to: #4643
Relates to: #4619
Relates to: npm/statusboard#403
@lukekarrys lukekarrys merged commit 6253d19 into npm:latest Apr 19, 2022
@ruyadorno ruyadorno mentioned this pull request Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: next These items should be addressed in the next release Release 8.x work is associated with a specific npm 8 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] npx/npm exec don't work in child workspace when using --no/--no-install and --package
3 participants