Prioritise popular transitive dependencies when hoisting

[sebmck]

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

[sebmck]

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

[bestander]

Do you need to seed the patterns again here?

[fingermark]

Will this help #570 at all?

[sebmck]

@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]

The CI should probably be fixed with a rebase

[sebmck]

All green! Merging!

[bestander]

Awesome, let's cherry-pick it into 0.21.1

[rally25rs]

I think this fixes #2673 ???

[bestander]

Yes it should

[markstos]

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

[bestander]

No, this step takes place after all dependencies are resolved and downloaded. There is a recent commit that fixed a bug though when several compatible entries in lockfile were merging. It was happening anyway but was not reflected in lockfile save

…

On Fri, 7 Apr 2017 at 19:42, Mark Stosberg ***@***.***> wrote: Does this change imply that different yarn.lock file structures will be generated after this patch is landed? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2676 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWF4mT2eg1UWfpeAyaQZl1bzPCBKAks5rtoOngaJpZM4L8SC7> .

[jesstelford]

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]

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]

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]

Thank you for clarifying. Yarn and npm don't have a feature to guarantee hoisting structure, this particular limitation seems quite rare to me. I would suggest to work around this at the level of fe-build or your application so that if fe-build/eslint got hoisted it would not break. From the top of my head, add eslint >2 to your application dependencies, then fe-build/eslint would not be hoisted as long as it is 2.0.

…

On Thu, 13 Apr 2017 at 00:39, Jess Telford ***@***.***> wrote: 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. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2676 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWCWcJeFrjz-Ai6_c4Dtrag2e_CGuks5rvWDFgaJpZM4L8SC7> .