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

Scoped approach to packages’ dependencies graph #4

Open
yuchi opened this issue Nov 25, 2016 · 7 comments
Open

Scoped approach to packages’ dependencies graph #4

yuchi opened this issue Nov 25, 2016 · 7 comments

Comments

@yuchi
Copy link
Owner

yuchi commented Nov 25, 2016

There’s one main design issue with the current implementation.
Packages are aggressively deduped, way beyond of what npm v3 does, and it’s less predictable than yarn forcefully flat dependencies layout.

Let me show an example.

Given the existence of these fake packages versions and their dependencies:

  • My bundle’s package, that depends on
    • angular ^1.0.0, that depends on
      • incremental-dom ^2.0.0
    • metal ^3.0.0, that depends on
      • incremental-dom ^2.0.0
    • incremental-dom ^4.0.0

In npm v3 we’d expect a layout that gives different instances for metal and angular:

my-bundle-web
└── #node_modules
    ├─• angular v1.0.0
    │   └── #node_modules
    │       └─• incremental-dom v2.0.0 # duplicate
    ├─• incremental-dom v4.0.0
    └─• metal v3.0.0
        └── #node_modules
            └─• incremental-dom v2.0.0 # duplicate

In yarn with --flat we’d expect a single version available to the whole project:

my-bundle-web
└── #node_modules
    ├─• angular v1.0.0
    ├─• incremental-dom v4.0.0 # !!
    └─• metal v3.0.0

Neither are the case now. Since we have a flat registry of versioned packages it is pretty trivial for us to bind every package to the highest version available for a dependency. This is pretty cool: every package has access to what it needs, yet we dedupe as much as we can.

The problem is that if angular does something nasty to incremental-dom (such as tampering some prototype or installing some plugin) that means that the tampered version is offered to metal too. This is not inherently bad, simply different from the expected behaviour and could be the cause of bugs and issues incredibly difficult to understand.


To be completely honest I believe a similar issues exists with npm.

Here’s an example:

  • My bundle’s package, that depends on
    • extract-cat-names ^1.0.0, that depends on
      • underscore ^1.0.0
      • underscore.strings ^1.0.0
    • calculate-alien-age ^1.0.0, that depends on
      • underscore ^1.0.0
      • underscore.strings ^2.0.0

In npm v3 we get this:

my-bundle-web/
└── #node_modules
    ├── calculate-alien-age v1.0.0
    │   └── #node_modules
    │       └── underscore.strings v2.0.0 # isolated
    ├── extract-cat-names v1.0.0
    ├── underscore v1.0.0
    └── underscore.strings v1.0.0

Which one gets moved to the root depends simply on the alphabetical order.

But if underscore.string is used as such:

var _ = require('underscore');
_.mixin(require('underscore.strings'));

That means that which one ends being called is the last one in the execution order (_.mixin adds methods to _) and for sure one between calculate-alien-age and extract-cat-names will get the wrong version.

@yuchi yuchi changed the title Deterministic approach to packages’ dependencies graph Scoped approach to packages’ dependencies graph Nov 25, 2016
@yuchi
Copy link
Owner Author

yuchi commented Jan 5, 2017

It took some time but I finally found evidence of this case in the wild without the need to resort to edge cases for example.

Take a look at the current compatibility matrix of react-redux. In its peerDependencies it lists this:

  "version": "5.0.1",
  // ...
  "peerDependencies": {
    "react": "^0.14.0 || ^15.0.0-0",
    "redux": "^2.0.0 || ^3.0.0"
  }

This means that the current superflat approach would make thins hairy to debug in this normal case (full dependency graph shown here):

  • my-old-app (bundle)
    • depends on react ^14.0.0
    • depends on redux ^3.0.0
    • depends on react-redux ^5.0.0
  • my-newer-app (bundle)
    • depends on react ^15.0.0
    • depends on redux ^2.0.0
    • depends on react-redux ^5.0.0
  • my-super-new-app (bundle)
    • depends on react ^15.0.0
    • depends on redux ^3.0.0
    • depends on react-redux ^5.0.0

While react/14.0.0 and redux/3.0.0 are actually a shared dependency and should rightfully merge in a single resolve→load→execute cycle, react-redux/5.0.0 requires to access the right react and redux relative to who is calling it.


A possible solution is to fully qualify packages with peerDependencies with also the resolved versions. Se when I resolve react-redux/lib/something inside my-super-new-app it does the following:

  1. identify the request as the module lib/something inside the package react-redux v3.0.0
  2. resolve the module as react-redux/3.0.0/~/lib/something.js (following rules in Reaching perfect Node-style resolution without burden #5)
  3. ensure the module and all its dependencies are loaded
  4. since the package has peer dependencies, retrieve the version of the packages from the requester, therefore extracts from my-super-new-app
    • react v15.0.0
    • redux v3.0.0
  5. resolve the qualifier the module as react-redux/3.0.0/{react/15.0.0,redux/3.0.0}/lib/something.js
  6. search for exports cached as that qualifier, otherwise execute the module and cache it
  7. profit

@jbalsas
Copy link

jbalsas commented Jan 18, 2017

Hey @yuchi, we're actually looking to see if we can switch from npm to yarn since the former is killing our build... would this make a difference?

@yuchi
Copy link
Owner Author

yuchi commented Jan 19, 2017

The main way yarn could help us is “flat mode” where multiple dependencies versions are forcefully flattened to the higher available version. This is Definitely Non-Conservative™.

We could have the same approach, either

  1. globally — you get the highest version available in the portal — or
  2. scoped by bundle — in the bundle package graph you get only the highest version available.

In case (1) we will encounter collisions and side effects. Read «installing a bundle is not safe».
In case (2) we have to rethink the strategy exposed in this thread, but could alleviate some problems.

There’s another way we could benefit from forcing yarn (or another *pm CLI) instead of npm, thats having a file structure simpler to manage than node_modules. For example jspm follows very closely the needs we have of a flat modules directory with versioned packages. Yarn is not far from giving this to us (they have it already in their cache, which is friendlier to use then npm’s) but they are not there yet.

@yuchi
Copy link
Owner Author

yuchi commented Feb 28, 2017

Let me make it even clearer, since I personally believe this is the biggest stopper here and must be well understood by everyone on board.

If I have two bundles which requires different versions of react but the same version of react-redux (which works with both) in one of the two you would get this:

const reactRedux = require('react-redux');
const React = require('react');
const Component = React.Component;

// This throws
console.assert(reactRedux.connect( …… )( …… ) instanceof Component);

@yuchi
Copy link
Owner Author

yuchi commented Mar 3, 2017

You can now have fun with a toy implementation of a resolution algorithm which supports the expected peerDependencies resolution: https://github.com/yuchi/liferay-ng-loader-experiments

Just clone, npm install, node index.js.

As you’ll see having as a peer-dependency a package which has peerDependencies doesn’t work.

@yuchi
Copy link
Owner Author

yuchi commented Mar 7, 2017

I updated that Proof of Concept a lot:

  • supports deep peer dependencies graphs;
  • identifies ciclic (recursive and unresolvable) peer dependencies;
  • has snapshot based tests;
  • nice errors are thrown here and there.

Please have a look and let me know if something is not clear.

@julien
Copy link
Collaborator

julien commented Mar 7, 2017

Hi @yuchi,

Thanks for the update. I'm currently viewing the changes and will let you know if I have any questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants