search

npm / rfcs

Public change requests/proposals & ideation
Other
730 stars 241 forks source link

[RRFC] Issue regarding npm-v7 peer-dependency behavior as described by RFC-0025-install-peer-deps and currently implemented in arborist and npm-v7-beta #204

Open KilianKilmister opened 4 years ago

KilianKilmister commented 4 years ago

UPDATE:

Full-fledged RFC: link PR has been filed: (#210) POC implementation in arborist fork: link

Any further discussion should be situated un those. getting ready to close this issue

The Issue

UPDATE: RFC

First Up

I'm generally very much in favour of what RFC-0025 proposes. This should not be taken as an argument against the proposal, but as a vital amendment to it.

Example

The way I see it, there are generally 3 major issues with peer-dependencies.

  1. projects with conservatively low set upper version limits
  2. arborits current semver-checks will not match any dev/beta version even if no upper limit (or no limit at all with *) is set.
  3. It could add to dependancy-hell by locking a package consumer into outdated versions of a package

Now this has been a personal annoyance since i started with node, and all i coud do about it is ignore it. But in the current (v7.0.0-beta.4) npm v7-beta, it causes arborist to throw an error during npm update. (it probably affects more than just that, but this is the issue i ran into just now) Companions: issue in npm/cli and issue in arborist

As mentioned, i think RFC-0025 is confronting a real issue, but i must imagine there is a considerable amount of people who would face similar issues as i am.

To Expand on my Issue

Running an npm-v6 install in the project i have currently opened (which is very representative of my projects) will result in this message:

npm WARN tsutils@3.17.1 requires a peer of typescript@>=2.8.0 || >= 3.2.0-dev || >= 3.3.0-dev || >= 3.4.0-dev || >= 3.5.0-dev || >= 3.6.0-dev || >= 3.6.0-beta || >= 3.7.0-dev || >= 3.7.0-beta but none is installed. You must install peer dependencies yourself. npm WARN ts-node@8.10.2 requires a peer of typescript@>=2.7 but none is installed. You must install peer dependencies yourself. npm WARN @typescript-eslint/eslint-plugin@2.34.0 requires a peer of eslint@^5.0.0 || ^6.0.0 but none is installed. You must install peer dependencies yourself. npm WARN @typescript-eslint/parser@2.34.0 requires a peer of eslint@^5.0.0 || ^6.0.0 but none is installed. You must install peer dependencies yourself. npm WARN eslint-plugin-react@7.14.3 requires a peer of eslint@^3.0.0 || ^4.0.0 || ^5.0.0 || ^6.0.0 but none is installed. You must install peer dependencies yourself. npm WARN eslint-plugin-import@2.18.2 requires a peer of eslint@2.x- 6.x but none is installed. You must install peer dependencies yourself.

Going over them

But what all of them have in common: I never had an issue with not meeting their peer deps, and have been using them extensively and for various edge-cases.

The absurdity

Now it might sound like this is a project with many dependancys. But on the contrary. Below are all the deps listed in my package.json. It's a whooping 13, only two of which are used at runtime. i essentially have 50% (6 of 13) of installed modules complain about peer-deps, with non of them actually having a valid reason for it and none of them even used at runtime, so it's only local, and does't affect any users of the package. It's straight up insanity.

{
  "devDependencies": {
    "@types/node": "^14.0.27",
    "@typescript-eslint/eslint-plugin": "^2.34.0",
    "@typescript-eslint/parser": "^2.34.0",
    "babel-eslint": "^10.1.0",
    "eslint": "^7.7.0",
    "standardx": "^5.0.0",
    "ts-node": "^8.10.2",
    "ts-typing-util": "file:../GitHub/ts-typing-util",
    "tsutils": "^3.17.1",
    "type-fest": "^0.16.0",
    "typescript": "^4.0.0-beta"
  },
  "dependencies": {
    "@repeaterjs/repeater": "^3.0.3",
    "reflect-metadata": "^0.1.13"
  }
}

concluding

Thus far it's only been a (quite frankly not so) minor neusance. But with unresolved peers now causing issues with project-management, this has become a major problem for me. Now the eslint thing isn't that much of a problem, altough i would very much like to use the latest versions of it when i'm using it programmatically instead of being forced to use a version that's 6 months and a semver-Major behind.

The real issue is with typescript. My codebases always use dev-versions, and because all my work is in next-gen and frontier stuff, i really depend on it to stay ontop and ahead of things.

Suggested Changes

In my personal opinion, there has to be some sort override that can be aplied on peer-deps (of both regular- and dev-deps) Imagine some fully functional and relatively popular, but badly maintained module has a peer-dep version range in which a major security vulnerability was found. It might be perfectly functional to use it with the updated version of the peer-dep, but the repo is abandoned and doesn't get the PRs merged. (inactive repos like that sadly arent uncommon and the amount of potential issues they can cause will only increase over time)

Specificly adressing RFC-0025

I very much agree that the behaviour described should be the default. I believe it will improve the eco-system and will make it easier for people unfamiliar with node to understand what's going on. The current state of peer deps is a regular point of confusion, not just for newcomers, but still very much for a large number of active developers. Because as mentioned in the RFC, peer deps have regularly been used in an optional fashion or migrated to proper (optional-)dep because of user-confusion.

This paragraph is the one i have a massive problem with:

If D and P cannot be placed in the tree in the presence of the newly requested dependency, then refuse to install it until the user resolves the conflict. Otherwise, move D and P to their new homes as part of the installation.

As this behaviour would affect every project i worked on in the past 6 months, and without a way to override it, could make it impossible to use the current core of my dev-tools.

What I would suggest is somewhat similar to Alternative F (which in itself is less of an alternative as it is an amendment). I don't think that the author should specify which peer-deps get auto installed and which don't. This could possibly lead to similar user-confusion and resulting (re-)moval of peer-deps as we are seing it now. I think the descision to include a peer-dep should be declarative, and being able to make a peer-dep defacto optional defeats the purpouse, as it could just aswell be filed as an optional dep with a not in the projects readme.

Instead i would suggest to give the package-consumer the possibility to override their dependencies peers in the their package.json.

Implementation

I can imagine some serious issues in module resolution if a peer-dep could simply be flagged to be ignored by arborist. Instead i think this needs to be implemented in a way where arborist is handed an alternative module that it will treated as a valid peer by design.

From the top of my head i can think of a few ways to implement this using my deps as an example: (i'm using the same key as in Alternative F just because. it could be whatever)

// the consumers package.json
{
  "devDependencies": {
    "ts-node": "^8.10.1", // has a peer of typescript@>=2.7
    "typescript": "^4.1.0-dev.20200815" // doesn't pass the semver-check because is a prerelease
  },
  "peerDependenciesMeta": {
    // tell arborist to settle for the locally install version
    "ts-node": { "typescript": "local" }
    // a hard override
    "tsutils": { "typescript": "^4.0.0-beta" } // possibly allow all the usual install specifiers like 'path' etc.
    // offer a substitute package
    "@typescript-eslint/parser": { 
      "eslint": { "standardx": "^5.0.0" }
    }
  }
}

NOTE: this should only affect module resolution. All changes caused by this should be the sole responsibility of the person who wants to override a peer-dep. the packages specified/linked would just be treated as a regular (dev-)dep and installed with the package (if the peer belongs to a regular dep with the finished package, if it belongs to a dev-dep then together with the other dev-deps)., so subsequent consumers would not have to worry about (or even have to be aware of) these mapping

Benefits

In my opinion this does come with a couple of advantages:

Conclusion

I believe my suggested changes would not only solve potential issues of the original RFC, they also persue the same goal, while allowing for more freedom and potentially even increasing the creative possibilities. Additionally, the changes require minimal effort to implement and would be essentially unnoticable for the average end-user

References

Companion Issues

KilianKilmister commented 4 years ago

@isaacs I am very interested to hear your opinion about this, both as writer of the original RFC and creator of npm and i'm more than willing to write an RFC based on this letter

ljharb commented 4 years ago

@KilianKilmister

It could add to dependancy-hell by locking a package consumer into outdated versions of a package

Peer dep ranges are an explicit contract that dependencies set. If you're not able to meet them, you have an invalid dep graph and you shouldn't expect things to work. The solution is for you to wait to upgrade the peer dep until all package authors have updated their packages to explicitly allow the new peer dep version.

As the maintainer of a number of packages that impose peer dep constraints, and that are themselves used as peer deps, I am strongly against anything that makes it easier for users to end up with an invalid dependency graph, as that increases the amount of bug reports I'll get from users who don't realize they're in an explicitly unsupported scenario.

KilianKilmister commented 4 years ago

@ljharb

I'll get from users who don't realize they're in an explicitly unsupported scenario.

Understandable, that's why i think it should be as clear as possible what the potential consequences of overriding peer-deps is. How about npm prompting a warn message similarly to how node warns of experimental features? Something like:

npm WARN has overridden peer dependencies and might not work correctly as a result.

This messsage would only have to be present if the peer-override is at the root-package, since in nested deps, malfunction because of overrrides would point to that package, and not to the peer of it's dep

I am strongly against anything that makes it easier for users to end up with an invalid dependency graph [...]

The usage of "anything" implies that you think it should not at all be possible to use an override. Is that correct or would there be an acceptable solution to this? (short of creating a fork and changing the package.json, which in my opinion leads to an unreasonable amount of work with having to actually maintain that fork, and pretty much defeats the purpous of a package manager)

The solution is for you to wait to upgrade the peer dep until all package authors have updated their packages to explicitly allow the new peer dep version.

Why this is not always a solution is adressed in this is adressed in this paragraph

Imagine some fully functional and relatively popular, but badly maintained module has a peer-dep version range in which a major security vulnerability was found. It might be perfectly functional to use it with the updated version of the peer-dep, but the repo is abandoned and doesn't get the PRs merged. (inactive repos like that sadly arent uncommon and the amount of potential issues they can cause will only increase over time)

This is the exact reason why i'm proposing this. packages that are badly maintained won't do that in an acceptable timeframe, unmaintained packages won't do this at all. And very few packages will ever include pre-release version. This effectively means i can't use npm-v7, and that would really suck.

An example i litteraly just now ran into with @rollup/plugin-typescript:

typescript@4.1 will be released in november, wich is a real problem for me:

And the plugin does actually support overrides (quote from plugin options)

typescript

Type: import('typescript')
Default: peer dependency

Overrides the TypeScript module used for transpilation.

typescript({
 typescript: require('some-fork-of-typescript')
});
ljharb commented 4 years ago

In that case, you already explicitly chose this problem by using a prerelease version of TypeScript. @rollup/plugin-typescript could certainly be updated to also allow v4 of TS, but until it is, this is just the world you've found yourself in by being an early adopter, I'm afraid.

Overrides are certainly something you should be able to do, and I believe https://github.com/npm/rfcs/pull/129 would provide that.

KilianKilmister commented 4 years ago

@ljharb

In that case, you already explicitly chose this problem by using a prerelease version of TypeScript. @rollup/plugin-typescript could certainly be updated to also allow v4 of TS, but until it is, this is just the world you've found yourself in by being an early adopter, I'm afraid.

multiple issues i have with this statement:

Overrides are certainly something you should be able to do, and I believe #129 would provide that.

I haven't looked at the open PR RFCs, i'll check it out I'm doing some experimenting about overrides on the ``acceptDependencies``` package field i found while examining the source. it's from this this (closed) PR as this is implemented in arborist

ljharb commented 4 years ago

no i didn't choose this. i chose to accept facing potential issues that come with that

Agreed, this is a more precise phrasing.

It certainly does seem like >=3.4.0 (what the rollup plugin requires) should include a v4 prerelease, at least.

KilianKilmister commented 4 years ago

Update: i started a preliminary RFC and i'm working on an basic implementation.

regarding #129: This is a much more complex implementation. My proposal tries to be as simple as possible so it could be implemented when v7 drops and not down the line, as is the plan for overrides

KilianKilmister commented 4 years ago

UPDATE:

Full-fledged RFC: link PR has been filed: (#210) POC implementation in arborist fork: link

Any further discussion should be situated un those. getting ready to close this issue

Pokute commented 2 years ago

I stumbled upon this same issue myself too.

I actually think that explicitly set dependencies to prerelease versions should satisfy peer dependencies automatically (given the other version number matches). Since a developer has deliberately and specifically added a dependency to a prerelease package, that developer shoulders the responsibility for the prerelease package working with the packages that have that peer dependency.

From semver.org:

A pre-release version indicates that the version is unstable and might not satisfy the intended compatibility requirements as denoted by its associated normal version.

This could very well be a non-breaking change. It's not like any existing build or project would suddenly and unexpectedly switch to a prerelease version even with the change.

The only risk I see is possibility of bug reports in libraries that have peer dependencies. Practically for libA that has a peer dependency of "libutil": "^1.2.3", there is a possibility for bug report that libA fails with libutil version 1.5.2-withuglyhack. That report however is easily dismissable as using an unsupported version.