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: reify git dependencies that have workspaces #103

Merged
merged 2 commits into from Feb 16, 2022

Conversation

nlf
Copy link
Contributor

@nlf nlf commented Feb 8, 2022

Draft until I write tests for this

This change will reify a git dependency if that git dependency has workspaces defined.

This is necessary because for git dependencies on a known hosting provider, we actually download an archive file rather than cloning the repository. That's all well and good, but tar refuses to unpack symlinks which is how a workspace tends to be stored in node_modules in a git repository that happens to have node_modules checked in. The end result today is that npm i -g npm/cli#release-next gives you a broken installation because all of the workspace modules are missing.

We could potentially extend the check so that it requires that the workspaces are defined and bundled dependencies are in use and a node_modules directory is present to be as specific as possible, but I'm not sure if that's overkill...

References

@ruyadorno ruyadorno force-pushed the nlf/fix-git-deps-with-workspaces branch from 3bd3801 to c62b8ec Compare February 15, 2022 22:10
@ruyadorno ruyadorno marked this pull request as ready for review February 15, 2022 22:10
@ruyadorno ruyadorno requested a review from a team as a code owner February 15, 2022 22:10
@ruyadorno
Copy link
Contributor

Rebased and added tests ✅

nlf and others added 2 commits February 15, 2022 17:14
When finding workspaces data in a package, pacote git fetcher should
extract contents and run prepare scripts as part of that, this test
makes sure that is happening.
@ruyadorno ruyadorno force-pushed the nlf/fix-git-deps-with-workspaces branch from c62b8ec to 75d70ee Compare February 15, 2022 22:14
@nlf
Copy link
Contributor Author

nlf commented Feb 15, 2022

i'm the original PR author so github won't let me click the approve button, but consider this approved 👍

@lukekarrys lukekarrys merged commit 08348fa into main Feb 16, 2022
@lukekarrys lukekarrys deleted the nlf/fix-git-deps-with-workspaces branch February 16, 2022 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants