Fix: Peer dependencies should only be looked up from top level dependencies #3803
Conversation
|
I deleted my comment because I was wrong ;) |
src/package-linker.js
Outdated
| return resolvedPattern ? this._satisfiesPeerDependency(range, resolvedPattern.version) : false; | ||
| const pkgs = this.resolver.getAllInfoForPackageName(name); | ||
| let foundPattern; | ||
| pkgs.find(pkg => { |
There was a problem hiding this comment.
I think what you intend to do is this:
const found = pkgs.find(pkg => {
const {root, version, patterns} = pkg._reference || {};
return root && this._satisfiesPeerDependency(range, version) && patterns;
});
if (found) {
ref.addDependencies(found._reference.patterns);
}
Modifying the outside scope in iteration callbacks are a bit dangerous and quite confusing to the reader :(
There was a problem hiding this comment.
yeah! i didn't like the side effect either but i got a warning about Property cannot be assigned on possibly undefined value then it got a bit messier as I had to check for found && found._reference, etc. I'll try with your suggestion again.
There was a problem hiding this comment.
You can probably use invariant() if nothing helps with that warning. That said, with the way I suggested, it shouldn't be a problem.
kaylie-alexa
force-pushed
the
peer-dependency
branch
from
4766ab9
to
960fff6
Compare
| }); | ||
| const foundPattern = found && found._reference && found._reference.patterns; |
There was a problem hiding this comment.
Not sure if this is you or flow yelling at you but you don't need to check found._reference since it is required for .find() to return something for found. Actually you can even move patterns up there and just check found as I suggested earlier. I'm guessing you did it this way because of flow complaining tho :D
There was a problem hiding this comment.
yup exactly, made flow unhappy. thanks!
BYK
changed the title
…4478) **Summary** Fixes #4446, fixes #4433, fixes #2688, fixes #2387. Follow up to #3803. The fix in #3893 was too aggressive, allowing only top-level dependencies to be used in peer dependency resolution which was incorrect. This patch allows resolving peer dependencies from the same or higher levels in the dependency tree. **Test plan** Additional unit and integration tests.
…arnpkg#4478) **Summary** Fixes yarnpkg#4446, fixes yarnpkg#4433, fixes yarnpkg#2688, fixes yarnpkg#2387. Follow up to yarnpkg#3803. The fix in yarnpkg#3893 was too aggressive, allowing only top-level dependencies to be used in peer dependency resolution which was incorrect. This patch allows resolving peer dependencies from the same or higher levels in the dependency tree. **Test plan** Additional unit and integration tests.
Summary
This is a bug fix for #3710
The previous implementation of satisfying peer dependencies of an added dependency looked through all the existing patterns of a dependency (including nested dependencies of other packages) and returned the first match, which ended up copying the nested modules of the other dependency and swallowing warnings about unmet/incorrect peer dependencies. This PR checks for whether the package is at root level or not and otherwise throws a warning if it doesn't meet the semver qualifications.
Test plan
Added tests, and tested on CLI that the peer dependencies don't get installed.
BEFORE
AFTER
