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

[BUG] Inconsistent dependancies versions for sub-dependancies due to deduplication #5202

Closed
2 tasks done
gmrchk opened this issue Jul 21, 2022 · 4 comments
Closed
2 tasks done
Labels
Bug thing that needs fixing Needs Triage needs review for next steps Release 8.x work is associated with a specific npm 8 release

Comments

@gmrchk
Copy link

gmrchk commented Jul 21, 2022

Is there an existing issue for this?

  • I have searched the existing issues

This issue exists in the latest npm version

  • I am using the latest npm

Current Behavior

Consider the following scenario (also available at https://github.com/gplopes/npm-dedupe-peer-deps for repro).

// main-app
"package-a": "file:../package-a",
"package-b": "file:../package-b",
"graphql": "15.0.0"

// package=a
 "graphql": "16.5.0",
 "@graphql-tools/schema": "8.3.8", // peerDependency: graphql "^14.0.0 || ^15.0.0 || ^16.0.0 || ^17.0.0"
 "@graphql-tools/stitch": "8.6.12" // peerDependency: graphql "^14.0.0 || ^15.0.0 || ^16.0.0 || ^17.0.0"

// package-b
"graphql": "15.0.0"

The npm install will produce following node_modules structure:

// node_modules (root)
  "graphql": "15.0.0" // deduped 
  "@graphql-tools/schema": "8.3.8"  // issue lays here where this package now references the root graphql which is a different version than the one install in package-a
  "@graphql-tools/stitch": "8.6.12"

└───project-a/node_modules
   └───"graphql": "16.5.0" // not deduped
   
└───project-b/node_modules
    └───"graphql": "15.0.0" // deduped (using root package)

@graphql-tools/schema uses the wrong graphql version from the root project (main-app).
The version of graphql for @graphql-tools/schema should be decided by the actual consumer of @graphql-tools/schema, which in this case is project-a, not main-app.

Expected Behavior

npm install should produce following node_modules stucture:

// node_modules (root)
│  "graphql": "15.0.0" // deduped 
│
└───project-a/node_modules
│   └───"graphql": "16.5.0"
│   └───"@graphql-tools/schema": "8.3.8" // should not be deduped as the project depends on graphql: 16.5.0
│   └───"@graphql-tools/stitch": "8.6.12" // should not be deduped as the project depends on graphql: 16.5.0
│   
└───project-b/node_modules
    └───"graphql": "15.0.0" // deduped (using root package)

Deduplication should consider the version of graphql installed in project-a to be the required version for its dependancies defining graphql as peer dependancy, like @graphql-tools/schema in this scenario.

Alternatively, sub dependancies with peer dependancies defined should not be deduped at all for version safety.

Steps To Reproduce

  1. Clone https://github.com/gplopes/npm-dedupe-peer-deps.
  2. Run npm i.
  3. Check the node_modules folder where @graphql-tools/schema and @graphql-tools/stitch are deduped to the root node_modules.

Environment

  • npm: 8.15.0
  • Node.js: 16.15.1
  • OS Name: MacOS Monterey
  • System Model Name: 12.4
  • npm config:
; "user" config from /Users/georgymarchuk/.npmrc

@pipedrive:registry = "https://npm-registry-proxy.pipedrive.tools" 
//npm-registry-proxy.pipedrive.tools/:_authToken = (protected) 
//npm-registry-proxy.pipedrive.tools/:always-auth = true 
//registry.npmjs.org/:_authToken = (protected) 

; node bin location = /Users/georgymarchuk/.nvm/versions/node/v16.15.1/bin/node
; node version = v16.15.1
; npm local prefix = /Users/georgymarchuk/git/npm-dedupe-peer-deps
; npm version = 8.15.0
; cwd = /Users/georgymarchuk/git/npm-dedupe-peer-deps
; HOME = /Users/georgymarchuk
; Run `npm config ls -l` to show all defaults.
@gmrchk gmrchk added Bug thing that needs fixing Needs Triage needs review for next steps Release 8.x work is associated with a specific npm 8 release labels Jul 21, 2022
@wraithgar
Copy link
Member

@graphql-tools/schema defines its peerDependencies as:

  "peerDependencies": {
    "graphql": "^14.0.0 || ^15.0.0 || ^16.0.0 || ^17.0.0"
  },

And since you already have 15.0.0 in your root, it's perfectly valid to hoist it.

How would npm know any differently? There is nothing in the setup of that tree that would inform npm that anything is wrong.

@gmrchk
Copy link
Author

gmrchk commented Jul 21, 2022

Because this way it leads to a situations where:

  • project-a uses graphql, and the version 16.5.0 is used.
  • project-a uses @graphql-tools/schema.
    • @graphql-tools/schema uses graphql under the hood, and since it's deduped, version 15.0.0 is used.

You're suddenly using two different versions of graphql package in a single script. Those can be two patch versions, but also two completely different major versions.
In some other situations where bundling would be involved, this would probably lead to unwanted multiple versions of packages being included in a bundle also.

Since project-a is the parent installing @graphql-tools/schema, and the @graphql-tools/schema expects graphql to be installed "by someone", the version of graphql for @graphql-tools/schema should be decided by it's direct parent (in this case project-a), otherwise you'll run into situations like the one above.

@wraithgar
Copy link
Member

That may seem intuitive to this situation but that doesn't always translate to it always being the most correct choice. A decision like this (to change how npm dedupes/hoists) needs to go through the rfc process. I would recommend opening either an RFC or an RRFC so that can be evaluated.

Here is one that is currently being actively discussed that is related to this issue, namely a way to inform npm of what packages have these sorts of conflicts. That may be relevant, or you may feel that this is a totally separate issue.

Closing this as a cli issue as the cli is currently working as designed. Again, please feel free to bring this up through the rfc process.

@gmrchk
Copy link
Author

gmrchk commented Jul 21, 2022

I've check the linked existing RFC, and I believe it's not directly related to the issue described. I will gladly open and RFC for this change. Thank you for the suggestion.

Before we move on to that, I think it needs to be said that the npm's dedupe seems like optimization, unless I'm mistaken.
However, in the case described, which is not unusual, it is essentially causing project-a to use a major version of graphql which is in no way compatible with the version defined in its package.json. This is effectively breaking the basic rules of versioning, and it's hard to believe it's conscious design decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Needs Triage needs review for next steps Release 8.x work is associated with a specific npm 8 release
Projects
None yet
Development

No branches or pull requests

2 participants