-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fixed: workspace entries should not be used when trying to check for peer dependencies #7812
Conversation
56934b5
to
0514589
Compare
|
0514589
to
bb65cf2
Compare
For what it's worth the failing test seems to be also failing on the master branch. |
src/package-linker.js
Outdated
} | ||
|
||
const {refTree, peerError, resolvedPeerPkg} = refTreesStatus[0]; | ||
if (resolvedPeerPkg) { | ||
ref.addDependencies(resolvedPeerPkg.patterns); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the first reference tree entry may see it's corresponding dependency added to the dependencies if resolved.
Other contributions will be discarded and only warnings will be issued if some of them are not resolved.
src/package-linker.js
Outdated
resolvedPeerPkg = peerPkgRef; | ||
} else { | ||
peerError = 'incorrectPeer'; | ||
const refTreesStatus = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We start by building an array containing the results on a per reference tree basis.
The first one would be the first reference tree found which (by construction) would be the first defined of the following:
- dependency defined at the root level
- first package (according to the platform's sorting conventions) which defines a dependency on that package
src/package-linker.js
Outdated
|
||
for (const peerDepName in peerDeps) { | ||
const range = peerDeps[peerDepName]; | ||
const meta = peerDepsMeta && peerDepsMeta[peerDepName]; | ||
|
||
const isOptional = !!(meta && meta.optional); | ||
|
||
const onNotResolved = isOptional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing to do with optional dependencies when they're not resolved.
src/package-linker.js
Outdated
minDistance = distance; | ||
} | ||
} | ||
const refTrees = extractRefTrees(ref); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we build an array of "reference trees" which would be sorted by "name" (the workspace's root reference tree carrying the empty name) and where each entry would represent the corresponding dependency path, instead of only taking the first one which was hanging around.
Just wondering if there was anything I should do here to proceed to the next stage (or to have it rejected with corresponding feedbacks for what it's worth). |
Hello @rally25rs Sorry for bothering you but I was wondering: is there anything I could do in order to ease the follow up on this PR ? I mean, is it even still relevant to have such a PR with yarn 2 "around the corner" ? Or did I miss something ? Thanks and all apologize for any inconvenience caused Regards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@antoine-malliarakis this is nice, thanks for the contribution! 👍
Tests pass locally, so I'm not sure what's up with CircleCI... I feel like any time I submit a PR they fail too 😆
I left a small comment on the test.
package-linker is feeling fairly cluttered, especially with adding another set of tree-traversal logic... if you wanted to put some extra effort into refactoring it off to it's own helper module that'd be nice, but overall 👍
I know with yarn v2 here we aren't making a lot of yarn v1 changes, but from the linked issue this seems like it's a pretty big/common annoyance so I'd be in favor of getting in merged.
@arcanis any additional thoughts?
bb65cf2
to
15665db
Compare
I'll let you decide, but to be honest I'm not sure it's a good idea to merge "significant" changes to the linker. I have no idea what side effect it may have 😕 I think the main problem I can see is that afaik this case wasn't considered when implementing workspaces (even in the 2.x I didn't think about making peer dependency implicitly define a regular dependency for workspaces), so it's possible that some parts of the application expect the workspace dependencies to be explicitly listed not only as peer dependencies, but also dev dependencies. |
(Oops sorry, didn't mean to close 😅) |
Would you rather, therefore, have this dev been done only on the v2 codeline ? (Would seem fair enough, to be honest) |
15665db
to
e89e37c
Compare
…kspaces virtual entries Fixed: the package requests introduced by workspace mechanism should be excluded from the scope of requests checked for usage Added peer tests (all credits go to @hulkish for the initial test cases)
e89e37c
to
5e22d2f
Compare
@arcanis Actually I just checked and it seems that there is no such issue in yarn 2... So there would be no real need to try "redoing it" on the 2.x codeline. |
Closing this MR as "out of touch" (lost track, no longer using yarn v1, and may have more impacts than foreseen) |
Fixed: the package requests introduced by workspace mechanism should be excluded from the scope of requests checked for usage
Added peer tests (all credits go to @hulkish for the initial test cases)
Summary
The idea of this PR is to fix the following behaviour which were currently giving tons of "false positive" warnings about unused peer dependencies in workspaces context.
Current state:
say you have a project A within your workspace which specifies a peer dependency on a package which is outside the workspace. Currently you get a peer unmet warning.
say you have a project B within your workspace which specifies a peer dependency on another project C which is inside the workspace and that your root project declares a dependency on B without a dependency on C. Currently you don't get a peer unmet warning.
This relates (somehow) to issue #5810
Test plan
Simple missing peer dependency warning (prod)
Create a root project with three subprojects,
a
,b
andc
(we'll be using version1.0
everywhere here).a
hasb
in its peer dependencies.c
hasa
in its prod dependenciesRun
yarn install
Expected: a warning for
c
missing a peer dependency onb
.Simple missing peer dependency warning (dev)
Create a root project with three subprojects,
a
,b
andc
(we'll be using version1.0
everywhere here).a
hasb
in its peer dependencies.c
hasa
in its dev dependenciesRun
yarn install
Expected: a warning for
c
missing a peer dependency onb
.Simple unmet peer dependency warning (prod)
Create a root project with three subprojects,
a
,b
andc
(we'll be using version1.0
everywhere here).a
hasb@0.5
in its peer dependencies.c
hasa
andb@1.0
in its prod dependenciesRun
yarn install
Expected: a warning for
c
with an unmet peer dependency onb@0.5
.Simple unmet peer dependency warning (dev)
Create a root project with three subprojects,
a
,b
andc
(we'll be using version1.0
everywhere here).a
hasb@0.5
in its peer dependencies.c
hasa
andb@1.0
in its dev dependenciesRun
yarn install
Expected: a warning for
c
with an unmet a peer dependency onb@0.5
.No peer dependency issue
Create a root project with three subprojects,
a
,b
andc
(we'll be using version1.0
everywhere here).a
hasb
in its peer dependencies.c
hasa
andb
in its prod dependenciesRun
yarn install
Expected: no warning