Correctly install workspace child deps when workspace child not symlinked to root #7289
Conversation
|
|
|
||
| // 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) { |
There was a problem hiding this comment.
Where is the second part of the condition ([...] which already have a different version in the root)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I also updated the comment to describe it better.
|
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. |
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 🙂 |
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.
|
Thanks @danez! Release should be out soon. |
…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
…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
…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
This fixes #4289.
Problem
The scenario is the following:
awith version2.0.0a@2.0.0has a dependency onb@2.0.0a@1.0.0andb@1.0.0(both coming from the registry)a@1.0.0andbdo not have dependencies.When install is called the hoister realizes it cannot hoist
a@2.0.0(the ws child) to the root becausea@1.0.0is already installed there. The path for the dependencyb@2.0.0will 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 fromto
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.