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

Prevents hoisting through peer dependencies #4086

Merged
merged 4 commits into from
Aug 7, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions __tests__/commands/install/integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@ async function mockConstants(base: Config, mocks: Object, cb: (config: Config) =
beforeEach(request.__resetAuthedRequests);
afterEach(request.__resetAuthedRequests);

test.concurrent('install should not hoist packages above their peer dependencies', async () => {
await runInstall({}, 'install-should-not-hoist-through-peer-deps', async (config): Promise<void> => {
expect(await fs.exists(`${config.cwd}/node_modules/a/node_modules/c`)).toEqual(true);
});
});

test.concurrent('install optional subdependencies by default', async () => {
await runInstall({}, 'install-optional-dependencies', async (config): Promise<void> => {
expect(await fs.exists(`${config.cwd}/node_modules/dep-b`)).toEqual(true);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
require("c")
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"name":"a", "version":"1.0.0", "dependencies":{"b":"file:../b-2", "c":"file:../c"}}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"name":"b", "version":"1.0.0"}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"name":"b", "version":"2.0.0"}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
console.log(require("b/package.json").version)
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"name":"c", "version":"1.0.0", "peerDependencies":{"b":"2.0.0"}}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"name":"d", "version":"1.0.0", "dependencies":{"b":"file:../b-1"}}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"name":"e", "version":"1.0.0", "dependencies":{"b":"file:../b-1"}}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
require("a")
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"dependencies":{"a":"file:./a", "d":"file:./d", "e":"file:./e"}}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
# yarn lockfile v1


"a@file:./a":
version "1.0.0"
dependencies:
b "file:../../../Users/mael/Library/Caches/Yarn/v1/b-2"
c "file:../../../Users/mael/Library/Caches/Yarn/v1/c"

"b@file:b-1":
version "1.0.0"

"b@file:b-2":
version "2.0.0"

"c@file:c":
version "1.0.0"

"d@file:./d":
version "1.0.0"
dependencies:
b "file:../../../Users/mael/Library/Caches/Yarn/v1/b-1"

"e@file:./e":
version "1.0.0"
dependencies:
b "file:../../../Users/mael/Library/Caches/Yarn/v1/b-1"
56 changes: 53 additions & 3 deletions src/package-hoister.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,41 @@ export default class PackageHoister {
return sortAlpha(aPattern, bPattern);
});

for (const [pattern, parent] of queue) {
// sort the queue again to hoist packages without peer dependencies first
let sortedQueue = [];
const availableSet = new Set();

let hasChanged = true;
while (queue.length > 0 && hasChanged) {
hasChanged = false;

for (let t = 0; t < queue.length; ++t) {
const pattern = queue[t][0];
const pkg = this.resolver.getStrictResolvedPattern(pattern);

const peerDependencies = Object.keys(pkg.peerDependencies || {});
const areDependenciesFulfilled = peerDependencies.every(peerDependency => availableSet.has(peerDependency));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, one needs to iterate on the dependencies to know if they have peer dependencies, so it can't really be optimized either. Eh. 😞

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (areDependenciesFulfilled) {
// Move the package inside our sorted queue
sortedQueue.push(queue[t]);
queue.splice(t--, 1);

// Add it to our set, so that we know it is available
availableSet.add(pattern);

// Schedule a next pass, in case other packages had peer dependencies on this one
hasChanged = true;
}
}
}

// We might end up with some packages left in the queue, that have not been sorted. We reach this codepath if two
// packages have a cyclic dependency, or if the peer dependency is provided by a parent package. In these case,
// nothing we can do, so we just add all of these packages to the end of the sorted queue.
sortedQueue = sortedQueue.concat(queue);

for (const [pattern, parent] of sortedQueue) {
const info = this._seed(pattern, {isDirectRequire: false, parent});
if (info) {
this.hoist(info);
Expand Down Expand Up @@ -259,13 +293,13 @@ export default class PackageHoister {
const stack = []; // stack of removed parts
const name = parts.pop();

//
for (let i = parts.length - 1; i >= 0; i--) {
const checkParts = parts.slice(0, i).concat(name);
const checkKey = this.implodeKey(checkParts);
info.addHistory(`Looked at ${checkKey} for a match`);

const existing = this.tree.get(checkKey);

if (existing) {
if (existing.loc === info.loc) {
// switch to non ignored if earlier deduped version was ignored (must be compatible)
Expand All @@ -291,8 +325,24 @@ export default class PackageHoister {
}
}

const peerDependencies = Object.keys(info.pkg.peerDependencies || {});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be we need a helper function, getPeerDependencies since I see this Object.keys(pkg.peerDependencies || {}) in multiple places now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// remove redundant parts that wont collide
while (parts.length) {
hoistLoop: while (parts.length) {
// we must not hoist a package higher than its peer dependencies
for (const peerDependency of peerDependencies) {
const checkParts = parts.concat(peerDependency);
const checkKey = this.implodeKey(checkParts);
info.addHistory(`Looked at ${checkKey} for a peer dependency match`);

const existing = this.tree.get(checkKey);

if (existing) {
info.addHistory(`Found a peer dependency requirement at ${checkKey}`);
break hoistLoop;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It almost looks like this part should become a separate method to avoid this label usage.

Copy link
Member Author

@arcanis arcanis Aug 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

const checkParts = parts.concat(name);
const checkKey = this.implodeKey(checkParts);

Expand Down