Prevents hoisting through peer dependencies #4086
Conversation
|
K, I think I got it. Seems like the issue is the order the dependencies are processed. We currently sort them by name, so in some case we might do the check "does the peer dependency exists?" before the peer dependency has been traversed. Only issue with this theory is that |
|
Aaaah found it, the compared patterns are |
|
Just pushed a fix that makes sure we're hoisting each package only once its peer dependencies have been hoisted themselves, recursively. Only remains the linting. ping @CrabDude, what do you think? |
src/package-hoister.js
Outdated
| let sortedQueue = []; | ||
| const availableSet = new Set(); | ||
|
|
||
| for (let hasChanged = true; queue.length > 0 && hasChanged;) { |
| const pkg = this.resolver.getStrictResolvedPattern(pattern); | ||
|
|
||
| const peerDependencies = Object.keys(pkg.peerDependencies || {}); | ||
| const areDependenciesFulfilled = peerDependencies.every(peerDependency => availableSet.has(peerDependency)); |
There was a problem hiding this comment.
This code inside the 2nd-level for loop looks a bit expensive to me. Do you think we can find more efficient ways to do this? Open for discussion.
There was a problem hiding this comment.
In theory it could be possible to avoid checking every fulfilled dependency every time, since we can just skip the ones we've already validated. In practice, the gain would be inconsequential and would depend on the engine optimizations more than anything else. Additionally, peer dependencies are rare enough for this code not to have a major (or even minor) impact on the perf. It's unlikely more than a single pass will be ever required.
That being said, there's maybe one optimization that can be useful, which is to just skip this whole codepath if there's no peer dependency (avoids us to do what is essentially a manual slice). Will do this.
There was a problem hiding this comment.
Nevermind, one needs to iterate on the dependencies to know if they have peer dependencies, so it can't really be optimized either. Eh. 😞
There was a problem hiding this comment.
Additionally, peer dependencies are rare enough for this code not to have a major (or even minor) impact on the perf. It's unlikely more than a single pass will be ever required.
I think this is a dangerous assumption in the long run since people are using peerDependencies more and more with many libraries as they are the "right way" for things like plugins etc.
| @@ -291,8 +326,24 @@ export default class PackageHoister { | |||
| } | |||
| } | |||
|
|
|||
| const peerDependencies = Object.keys(info.pkg.peerDependencies || {}); | |||
There was a problem hiding this comment.
May be we need a helper function, getPeerDependencies since I see this Object.keys(pkg.peerDependencies || {}) in multiple places now.
There was a problem hiding this comment.
I feel like the indirection would make the code less clear. Why make a function call to an helper when you can just call the function this helper calls? It only matters if we plan to change the info.pkg underlying type and suddenly it wasn't a mapping of the package.json anymore, but I don't see what we would have to gain in doing that.
There was a problem hiding this comment.
I feel like the indirection would make the code less clear.
It would actually make the code clearer since it gives a name to what you are doing and it is shorter: less cognitive overload. Also makes that part of the code separately testable and less prone to errors and easier to refactor since you hoisted up repeated code.
|
|
||
| if (existing) { | ||
| info.addHistory(`Found a peer dependency requirement at ${checkKey}`); | ||
| break hoistLoop; |
There was a problem hiding this comment.
It almost looks like this part should become a separate method to avoid this label usage.
There was a problem hiding this comment.
I don't know - despite being usually avoided, the label has a great use case here, and I feel like the hoister is complex enough to warrant some of its logic to be kept inline, so that we don't have to jump everywhere in the file to understand its process. There's already too many functions imo :p
There was a problem hiding this comment.
I don't know - despite being usually avoided, the label has a great use case here, and I feel like the hoister is complex enough to warrant some of its logic to be kept inline
With this line of thinking, we can increase complexity infinitely instead of trying to find simpler ways of doing things.
There's already too many functions imo :p
Too many functions >> too much code in a single function
|
The next to last appveyor build failed because of a Github 500, and the last one failed because of a segfault. Third time's the charm ... |
|
Will this end up in an upcoming release anytime soon? I see this was part of the |
Should fix #3933 - not yet ready, the simple reproduction script pass, but not the real-world case.