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

Correctly install workspace child deps when workspace child not symlinked to root #7289

Merged
merged 4 commits into from Nov 22, 2019

Conversation

@danez
Copy link
Contributor

@danez danez commented May 20, 2019

This fixes #4289.

Problem

The scenario is the following:

  • We have a workspace repository which has one workspace child named a with version 2.0.0
  • a@2.0.0 has a dependency on b@2.0.0
  • the root has a dependency on a@1.0.0 and b@1.0.0 (both coming from the registry)
  • a@1.0.0 and b do not have dependencies.

When install is called the hoister realizes it cannot hoist a@2.0.0 (the ws child) to the root because a@1.0.0 is already installed there. The path for the dependency b@2.0.0 will then be created as _root_/node_modules/workspace-aggregator-1234/node_modules/a/node_modules/b/ which is completely wrong. The linker then installs the dependency there.

In the tests the behavior is different for some reason and yarn errors with error "workspace-aggregator-1234#arrify#isarray" not installed. But anyway this needs to be fixed.

Proposed fix

The fix I went for is located in the package-hoister. The hoister creates the tree as usual but in init() where it creates a flat tree we check for each dependency if it is actually coming from within a ws child. If it does we change its destination from

_root_/node_modules/workspace-aggregator-1234/node_modules/_child_/node_modules/_dep_/

to

_root_/packages/_child_/node_modules/_dep_/

I also moved the skipping of workspace dependencies from the linker to the hoister, as I have to do the check now there anyway, because the change I described should not be applied to the virtual workspace or the workspace child itself. And imho it looks more correct to me to not even create a wrong path instead of creating it and then skipping it in the linker.

Note that if the dependency of the child is hoisted to the root it will not show up in the tree as subdependency for the ws child. So we don't have to worry about that case, it works exactly the same as now.

If you are worried about the move of the skipping I can readd it or move it back to the linker.

Test plan

I added a test case which fails without the fix.

As soon as CI will finish failing with the test-only commit I will submit the fix.

@buildsize
Copy link

@buildsize buildsize bot commented May 20, 2019

File name Previous Size New Size Change
yarn-[version].noarch.rpm 1.18 MB 1.18 MB 218 bytes (0%)
yarn-[version].js 4.86 MB 4.86 MB 927 bytes (0%)
yarn-legacy-[version].js 5.05 MB 5.05 MB 986 bytes (0%)
yarn-v[version].tar.gz 1.19 MB 1.19 MB 199 bytes (0%)
yarn_[version]all.deb 868.99 KB 869.34 KB 364 bytes (0%)

@danez danez marked this pull request as ready for review May 20, 2019
@danez danez force-pushed the fix-workspace-confilct branch 2 times, most recently from 2dffb3d to 7fee22e May 20, 2019
Copy link
Member

@arcanis arcanis left a comment

I wonder if the solution wouldn't be to just never hoist workspaces. It would create a few extra symlinks but it's not the end of the world 🤔


// we don't need to install the virtual manifest or workspace packages
// which already have a different version in the root
if (isWorkspaceEntry && keyParts.length <= 2) {
Copy link
Member

@arcanis arcanis May 24, 2019

Where is the second part of the condition ([...] which already have a different version in the root)?

Copy link
Contributor Author

@danez danez May 24, 2019

It is dependent on the length:

So basically it is this:

if (
  this.workspaceLayout && // Do we have a workspace?
  keyParts[0] === this.workspaceLayout.virtualManifestName && // Is the first keypart the virtual manifest name? If so we are dealing with modules from within workspaces or the virtual manifest itself
  (
    keyParts.length === 1 || // the key is "workspace-aggregator-1234" Which is the virtual workspace itself
    keyParts.length === 2  //  the key is "workspace-aggregator-1234#child" Which is one of the workspace childs which could not be hoisted.
  )
){}

If the workspace child was hoisted to the root the key would be child directly or respectively child#deps which can be installed correctly already, because child is symlinked in the root node_modules folder.

If the length is > 3 then we are dealing with dependencies of a ws child which was not hoisted to the root node_modules folder, which is exactly what we want to handle.

Copy link
Contributor Author

@danez danez May 24, 2019

I also updated the comment to describe it better.

@danez
Copy link
Contributor Author

@danez danez commented May 24, 2019

I'm not sure what exactly you mean by never hoisting workspaces, but if you imply that ws childs should never be symlinked in the root node_modules folder I would think this is a pretty big breaking change.

@danez danez force-pushed the fix-workspace-confilct branch from bbba2ac to 71ab421 May 24, 2019
@arcanis
Copy link
Member

@arcanis arcanis commented Nov 18, 2019

but if you imply that ws childs should never be symlinked in the root node_modules folder I would think this is a pretty big breaking change.

Yep that's what I was mentioning. It would be breaking indeed - although only for already invalid usages (since relying on packages that aren't part of your own dependencies is forbidden). Anyway, I think it's good to merge as it is - the next major will take care of the breaking part 🙂

danez added 4 commits Nov 21, 2019
This test creates the scenario where the child is not symlinked into the root node_modules folder, because
the root installs a different version of the workspace child from the registry.
@danez danez force-pushed the fix-workspace-confilct branch from ce6c231 to 7af159a Nov 21, 2019
@arcanis arcanis merged commit c43f66d into yarnpkg:master Nov 22, 2019
18 of 19 checks passed
@arcanis
Copy link
Member

@arcanis arcanis commented Nov 22, 2019

Thanks @danez! Release should be out soon.

arcanis added a commit that referenced this issue Nov 22, 2019
…nked to root (#7289)

* Test for conflict between pkg versions in root and workspace child

This test creates the scenario where the child is not symlinked into the root node_modules folder, because
the root installs a different version of the workspace child from the registry.

* fix(hoister): Correctly change path for ws child dependencies when child not hoisted

* Update CHANGELOG.md

* Better comment
VincentBailly pushed a commit to VincentBailly/yarn that referenced this issue Jun 10, 2020
…nked to root (yarnpkg#7289)

* Test for conflict between pkg versions in root and workspace child

This test creates the scenario where the child is not symlinked into the root node_modules folder, because
the root installs a different version of the workspace child from the registry.

* fix(hoister): Correctly change path for ws child dependencies when child not hoisted

* Update CHANGELOG.md

* Better comment
VincentBailly added a commit to VincentBailly/yarn that referenced this issue Jun 10, 2020
…nked to root (yarnpkg#7289)

* Test for conflict between pkg versions in root and workspace child

This test creates the scenario where the child is not symlinked into the root node_modules folder, because
the root installs a different version of the workspace child from the registry.

* fix(hoister): Correctly change path for ws child dependencies when child not hoisted

* Update CHANGELOG.md

* Better comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants