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(nm): Properly hoist nested workspaces #3438

Merged
merged 21 commits into from Oct 12, 2021
Merged

Conversation

larixer
Copy link
Member

@larixer larixer commented Sep 14, 2021

What's the problem this PR addresses?

Fixes #3429 by taking into account physical location on the filesystem of nested workspaces and not only their place in the dependency graph.

Avoids creating self-referencing symlinks for anonymous workspaces.

How did you fix it?

The place of nested workspaces on the file system are tracked now. The hoisting rule has been added to disallow workspace hoisting (because the workspace directory cannot be moved on the file system), while still allowing hoisting workspace dependencies freely.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@larixer larixer force-pushed the larixer/nm-nested-workspaces branch 2 times, most recently from 5e9bfb3 to a39c4cc Compare September 17, 2021 10:13
@larixer larixer marked this pull request as ready for review September 17, 2021 11:16
@larixer
Copy link
Member Author

larixer commented Sep 23, 2021

Noticed that anonymous workspaces became broken, back to draft

@larixer larixer marked this pull request as draft September 23, 2021 12:30
@larixer
Copy link
Member Author

larixer commented Sep 23, 2021

Noticed that anonymous workspaces became broken, back to draft

Disregard, I have confused Yarn with nested portals, I'm not sure how they should work at all

@larixer larixer marked this pull request as ready for review September 23, 2021 13:06
@larixer larixer marked this pull request as draft September 23, 2021 14:44
@larixer
Copy link
Member Author

larixer commented Sep 23, 2021

Noticed self-check violation in one of my projects, investigating...

@nicolo-ribaudo
Copy link
Contributor

@larixer It looks like this PR also fixes #3518; it might be worth to add a test.

@larixer
Copy link
Member Author

larixer commented Oct 3, 2021

@nicolo-ribaudo Yes, this PR solves various cases with workspace nesting, by taking into account their nesting on the file system, which might be very different from nesting in the dependency graph. It needs more testing, because changes are not trivial

@larixer larixer marked this pull request as ready for review October 5, 2021 11:29
@larixer
Copy link
Member Author

larixer commented Oct 5, 2021

@nicolo-ribaudo I have checked this PR over several repos I have in addition to the integration and E2E tests we have i n the code, all my repos were installed correctly. The PR is ready for review.

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Oct 7, 2021
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Oct 7, 2021
@arcanis arcanis merged commit ab203be into master Oct 12, 2021
@arcanis arcanis deleted the larixer/nm-nested-workspaces branch October 12, 2021 15:51
@larixer
Copy link
Member Author

larixer commented Oct 12, 2021

@nicolo-ribaudo Seems the commit referencing this PR from your repo is failing the CI. Do you know if the problem is on our side or on your side?

@nicolo-ribaudo
Copy link
Contributor

The failure is not related to this PR: it's either a bug in the constraints plugin or (way more likely) I did a mistake while rebasing that PR. I'll check later today.

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.

[Bug?]: node-modules - Dependency with different major version in nested workspace not installed
3 participants