fix: make sure hoistingLimits are not bypassed

[Knagis]

What's the problem this PR addresses?

Fixes issue when hoistingLimits: dependencies get broken due to the same dependency being both a direct one (thus isHoistBorder: true) and a transitive one (with isHoistBorder: false)

Resolves #6662

...

How did you fix it?

during cloneTree() mark the copied workNode as hoist border when it is such at any time in the tree, not just the first time it is encountered.

Consider tree:

• pkg1
  • pkg2
    • pkg3
• pkg2
  • pkg3

with hoistingLimits: dependencies both pkg1 and pkg2 on the root level must be hoist borders. but due to the depth-first approach of cloneTree, the first time pkg2 is encountered is was marked with isHoistBorder: false and that was applied even to the top level entry.

before the fix:

• pkg1 hoist border
  • pkg2
    • pkg3
• pkg2
  • pkg3

and this means that pkg3 gets hoisted to top level.

The fix makes sure that pkg2 is correctly marked as hoist border.

The fix is not ideal, it results in

• pkg1 hoist border
  • pkg2 hoist border
    • pkg3
• pkg2 hoist border
  • pkg3

While not ideal, the fix does ensure that pkg3 does not get hoisted to top level which is the main point of hoistingLimits: dependencies

Checklist

• I have read the Contributing Guide.

• 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.

[Knagis]

i might be mistaken, but it seems the tests fail on master as well..

at least

 ● Plugins › typescript › Adding types › it should automatically add @types for scoped packages

failed for me locally from master branch.

[larixer]

@Knagis Thanks for the fix. I'm worried that it might prevent hoisting in unexpected places though. hoistingLimits: dependencies is meant to disable hoisting on direct dependencies only, and not everywhere in the tree where such dependencies may occur. Your fix prevents hoisting everywhere in the tree where these dependencies might occur, which seems to make hoisting broken the other way, it seems. Correct me, if I am wrong.

[Knagis]

In my opinion the hoisting with the fix is not optimal, but it is correct.

Without the fix hoistingLimits: dependencies is very broken as it results in pkg3 being on the top level - exactly the thing the setting is supposed to block:

• pkg1 hoist border
• pkg2
• pkg3

With the fix - the setting is respected:

• pkg1 hoist border
  • pkg2 hoist border
    • pkg3
• pkg2 hoist border
  • pkg3

Nothing within pkg1 should be expecting pkg3 to be hoisted up one level - since hoisting is essentially optional. yarn pnp mode won't make pkg3 available to pkg1 etc.

---

The further improvements to this case are apparent here -

1. pkg1 -> pkg2 should not be hoist border
2. pkg2 can be "hoisted" over pkg1 hoist border - it isn't hoisting as much as it is deduplicating.

i tried a bit to get 1. done but couldn't come up with something elegant (mostly because of the self dependencies for each pkg), and as it would likely involve much bigger refactor of cloneTree, i think that could/should be separate PR.

[larixer]

@Knagis I understand, the fix though should be:

• pkg1 hoist border

  • pkg2
    • pkg3

• pkg2 hoist border

  • pkg3

  Otherwise it is not a fix, we just change one broken implementation with another. I see no point in doing so.

[Knagis]

Yarn documentation https://yarnpkg.com/configuration/yarnrc#nmHoistingLimits states

If dependencies, transitive dependencies also won't be hoisted past your direct dependencies.

Current behavior violates this rule.

The fix does not violate this rule.

Is there any other guarantee specified anywhere in terms of how hoisting works?

[larixer]

In my opinion the hoisting with the fix is not optimal, but it is correct.

Without the fix hoistingLimits: dependencies is very broken as it results in pkg3 being on the top level - exactly the thing the setting is supposed to block:

• pkg1 hoist border
• pkg2
• pkg3

With the fix - the setting is respected:

• pkg1 hoist border

  • pkg2 hoist border

    • pkg3

• pkg2 hoist border

  • pkg3

Nothing within pkg1 should be expecting pkg3 to be hoisted up one level - since hoisting is essentially optional. yarn pnp mode won't make pkg3 available to pkg1 etc.

The further improvements to this case are apparent here -

1. pkg1 -> pkg2 should not be hoist border
2. pkg2 can be "hoisted" over pkg1 hoist border - it isn't hoisting as much as it is deduplicating.

i tried a bit to get 1. done but couldn't come up with something elegant (mostly because of the self dependencies for each pkg), and as it would likely involve much bigger refactor of cloneTree, i think that could/should be separate PR.

Yes, I agree with the plan and with corner cases 1 and 2 you highlighted here. Okay, let's merge this PR as an intermediate improvement, since it trade offs broken implementation with non-optimal hoisting. The hoisting is considered an optional optimization and from this point of view your PR is fair.

[larixer]

The failing integration tests prevent me from merging this PR, it is technical limitation, since failing integration tests marked as "required". If the problems in failing integration tests are unrelated to this PR they should show up on other PRs, let's see if this will be the case.