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): improves external soft links hoisting #4269

Merged
merged 10 commits into from
Mar 28, 2022

Conversation

larixer
Copy link
Member

@larixer larixer commented Mar 25, 2022

What's the problem this PR addresses?

When all the dependencies were hoisted from nested portals the portals were not hoisted

Fixes: #4240

How did you fix it?

Applies additional hoisting rounds when external soft links run out of unhoisted dependencies

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 marked this pull request as draft March 25, 2022 18:31
@larixer larixer marked this pull request as ready for review March 26, 2022 12:07
packages/yarnpkg-nm/sources/hoist.ts Outdated Show resolved Hide resolved
packages/yarnpkg-nm/sources/hoist.ts Outdated Show resolved Hide resolved
larixer and others added 3 commits March 27, 2022 10:26
Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
@merceyz merceyz merged commit 7f64a98 into master Mar 28, 2022
@merceyz merceyz deleted the larixer/nm-imrpove-portal-hoisting branch March 28, 2022 00:05
merceyz pushed a commit that referenced this pull request May 12, 2022
* Improves portal hoisting

* Move hasUnhoistedDependencies into a separate function

* Updates test, removes debugging code

* Adds support and a test for deep nested portals hoisting

* Fixes test description

* Lint

* Update packages/yarnpkg-nm/sources/hoist.ts

Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>

* Update packages/yarnpkg-nm/sources/hoist.ts

Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>

* Lint

Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
@@ -668,6 +684,9 @@ const hoistGraph = (tree: HoisterWorkTree, rootNodePath: Array<HoisterWorkTree>,
}
}

if (parentNode.dependencyKind === HoisterDependencyKind.EXTERNAL_SOFT_LINK && wereNodesHoisted)
Copy link

Choose a reason for hiding this comment

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

Why only EXTERNAL_SOFT_LINK need anotherRound? I meet a bug that parentNode is a sub workspace and could not hoist its dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Please open an issue with a reproduction.

Copy link

Choose a reason for hiding this comment

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

#5332 here is the issue with a reproduction repo.

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?]: yarn link failing with "Writing attempt prevented to..."
4 participants