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

Prioritise popular transitive dependencies when hoisting #2676

Merged
merged 2 commits into from Feb 15, 2017

Conversation

Projects
None yet
6 participants
@kittens
Member

kittens commented Feb 9, 2017

This PR prioritises popular transitive dependencies when we perform hoisting. For example, currently if a dependency one level deep depends on a@1.0.0 then it's immediately hoisted to the top. If then two levels deep there are multiple dependencies relying on a@2.0.0 then the top position has already been taken.

Instead, with this patch, we first do a prepass over the dependency graph and hoist transitive dependencies that have multiple versions according to what one is depended on the most.

While testing this patch I used a new create-react-app project to test, the following are before and after sizes of node_modules:

Before: 478MB
After: 118MB
@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Feb 9, 2017

Member

Will look into the failures tomorrow. Just looks like I need to update a test that was impacted by this prioritization.

Member

kittens commented Feb 9, 2017

Will look into the failures tomorrow. Just looks like I need to update a test that was impacted by this prioritization.

@@ -87,6 +87,8 @@ export default class PackageHoister {
*/
seed(patterns: Array<string>) {
this.prepass(patterns);
for (const pattern of this.resolver.dedupePatterns(patterns)) {
this._seed(pattern);

This comment has been minimized.

@bestander

bestander Feb 10, 2017

Member

Do you need to seed the patterns again here?

@bestander

bestander Feb 10, 2017

Member

Do you need to seed the patterns again here?

@fingermark

This comment has been minimized.

Show comment
Hide comment
@fingermark

fingermark Feb 10, 2017

Will this help #570 at all?

fingermark commented Feb 10, 2017

Will this help #570 at all?

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Feb 10, 2017

Member

@fingermark It's not related. This is to do with transitive dependencies not being hoisted because of conflicts causing a lot of duplicated dependencies.

Member

kittens commented Feb 10, 2017

@fingermark It's not related. This is to do with transitive dependencies not being hoisted because of conflicts causing a lot of duplicated dependencies.

@bestander

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander Feb 14, 2017

Member

The CI should probably be fixed with a rebase

Member

bestander commented Feb 14, 2017

The CI should probably be fixed with a rebase

@ghost ghost referenced this pull request Feb 14, 2017

Closed

Optimize version conflict strategy #1150

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Feb 15, 2017

Member

All green! Merging!

Member

kittens commented Feb 15, 2017

All green! Merging!

@kittens kittens merged commit c77d4d7 into master Feb 15, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kittens kittens deleted the prioritise-popular branch Feb 15, 2017

@bestander

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander Feb 15, 2017

Member

Awesome, let's cherry-pick it into 0.21.1

Member

bestander commented Feb 15, 2017

Awesome, let's cherry-pick it into 0.21.1

@rally25rs

This comment has been minimized.

Show comment
Hide comment
@rally25rs

rally25rs Feb 21, 2017

Contributor

I think this fixes #2673 ???

Contributor

rally25rs commented Feb 21, 2017

I think this fixes #2673 ???

@bestander

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander Feb 21, 2017

Member

Yes it should

Member

bestander commented Feb 21, 2017

Yes it should

@markstos

This comment has been minimized.

Show comment
Hide comment
@markstos

markstos Apr 7, 2017

Contributor

Does this change imply that different yarn.lock file structures will be generated after this patch is landed?

Contributor

markstos commented Apr 7, 2017

Does this change imply that different yarn.lock file structures will be generated after this patch is landed?

@bestander

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander Apr 7, 2017

Member
Member

bestander commented Apr 7, 2017

@jesstelford

This comment has been minimized.

Show comment
Hide comment
@jesstelford

jesstelford Apr 12, 2017

Is there any downside with this change potentially causing unexpected dependency changes when relying on an implicit dependency?

For example;

At Domain.com.au, we have a centralized build tool which itself wraps other tools, named fe-build. We then use this tool as a devDependency for all (200+) of our React components (similarish to react-scripts)

my-component/package.json

{
  "devDependencies": {
    "fe-build": "^10.0.0"
  },
  "scripts": {
    "lint": "eslint"
  }
}

One of those dependencies might be a specific version of eslint:

fe-build/package.json

{
  "dependencies": {
    "eslint": "^2.0.0"
  }
}

We may also depend on a bunch of other things in my-component, each of which could conceivably bring in their own version of eslint (or whatever tool).

The previous algorithm would always grab one particular eslint to hoist (eg; the version depended on in fe-build: "eslint": "^2.0.0"). The developer could then confidently use yarn run lint to execute the expected version of eslint.

However, with this change, it could be the case that if other dependencies each specify "eslint": "^3.0.0", the version hoisted would no longer match the expected version for the developer. Ie; Running yarn run lint would execute eslint@3.x.x instead of eslint@2.x.x.

I'm not sure if this is an issue in reality, but I'd be worried this may crop up in the future as our dependency trees get bigger and bigger, especially when the pressures to deliver products outweigh updating dependencies in our build tool fe-build.

jesstelford commented Apr 12, 2017

Is there any downside with this change potentially causing unexpected dependency changes when relying on an implicit dependency?

For example;

At Domain.com.au, we have a centralized build tool which itself wraps other tools, named fe-build. We then use this tool as a devDependency for all (200+) of our React components (similarish to react-scripts)

my-component/package.json

{
  "devDependencies": {
    "fe-build": "^10.0.0"
  },
  "scripts": {
    "lint": "eslint"
  }
}

One of those dependencies might be a specific version of eslint:

fe-build/package.json

{
  "dependencies": {
    "eslint": "^2.0.0"
  }
}

We may also depend on a bunch of other things in my-component, each of which could conceivably bring in their own version of eslint (or whatever tool).

The previous algorithm would always grab one particular eslint to hoist (eg; the version depended on in fe-build: "eslint": "^2.0.0"). The developer could then confidently use yarn run lint to execute the expected version of eslint.

However, with this change, it could be the case that if other dependencies each specify "eslint": "^3.0.0", the version hoisted would no longer match the expected version for the developer. Ie; Running yarn run lint would execute eslint@3.x.x instead of eslint@2.x.x.

I'm not sure if this is an issue in reality, but I'd be worried this may crop up in the future as our dependency trees get bigger and bigger, especially when the pressures to deliver products outweigh updating dependencies in our build tool fe-build.

@bestander

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander Apr 12, 2017

Member

That should not be a problem.

If you want to run a specific version of eslint in your project then eslint needs to be a top level dependency, not an implicit one.
The hoisting algorithm will put most used versions of subdependencies on top but it will respect commonjs/node dependency resolution logic, so every package that depends on a specific version of eslint will always get the right one either in its direct node_modules or node_modules in one of the parents.

Member

bestander commented Apr 12, 2017

That should not be a problem.

If you want to run a specific version of eslint in your project then eslint needs to be a top level dependency, not an implicit one.
The hoisting algorithm will put most used versions of subdependencies on top but it will respect commonjs/node dependency resolution logic, so every package that depends on a specific version of eslint will always get the right one either in its direct node_modules or node_modules in one of the parents.

@jesstelford

This comment has been minimized.

Show comment
Hide comment
@jesstelford

jesstelford Apr 12, 2017

To clarify; fe-build in this example is relying on the behaviour of the old hoisting algorithm as part of its API. my-component purposely does not have a dependency on eslint, expecting it to be hoisted from node_modules/fe-build/node_modules/eslint into node_modules/eslint.

I understand this is somewhat akin to doing a require('foo/lib/some-internal-file.js') (relying on internal APIs), but until this point, that API has been the same and using it has worked well for us.

To be clear; I don't actually think this will come up, but if it did, it would be a hell of a thing to debug, and would cause a lot of head scratching for devs.

jesstelford commented Apr 12, 2017

To clarify; fe-build in this example is relying on the behaviour of the old hoisting algorithm as part of its API. my-component purposely does not have a dependency on eslint, expecting it to be hoisted from node_modules/fe-build/node_modules/eslint into node_modules/eslint.

I understand this is somewhat akin to doing a require('foo/lib/some-internal-file.js') (relying on internal APIs), but until this point, that API has been the same and using it has worked well for us.

To be clear; I don't actually think this will come up, but if it did, it would be a hell of a thing to debug, and would cause a lot of head scratching for devs.

@bestander

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander Apr 13, 2017

Member
Member

bestander commented Apr 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment