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

Overrides not checked with pnpm #78

Closed
TxHawks opened this issue May 30, 2022 · 9 comments
Closed

Overrides not checked with pnpm #78

TxHawks opened this issue May 30, 2022 · 9 comments

Comments

@TxHawks
Copy link

TxHawks commented May 30, 2022

Description

In a pnpm monorepo, I have a pnpm.overrieds field in the root package.json in order to override a an indirect dependency (a dependency of a dependency).

And then, in the same package.json devDependencies field or in one of the workspaces, is have an explicit dependency on eslint@^7.0.0.

// package.json
{
  // ...
  "devDependencies": {
    "eslint": "^7.0.0"
  },
  "pnpm:" {
    "overrides": {
      "eslint": "^8.16.0"
    }
  }
}

When I run syncpack list-mismatches, this discrepancy to be reported, but it isn't, even if I specifically add the --overrides flag.

A minimal reproduction repo is available here: https://github.com/TxHawks/syncpack-overrides-repro

Suggested Solution

Since overrides are mostly used to deal with indirect dependencies, where you have no direct control over the content of the package.json files, I'd expect list-mismatches to report this discrepancy like any other version mismatch.

Help Needed

If this is indeed a bug, I'd be happy to take a stab at this, and would appreciate pointers for where in the code to start looking at and maybe hash out the best way to do this.

@JamieMason
Copy link
Owner

Thanks for posting. I thought overrides was at .overrides and not .pnpm.overrides, if that's not the case then this is what will be wrong in syncpack.

@TxHawks
Copy link
Author

TxHawks commented May 30, 2022

Thanks for the quick reply. Overrides is indeed under pnpm. See here: https://pnpm.io/package_json#pnpmoverrides

@TxHawks
Copy link
Author

TxHawks commented May 30, 2022

Do you want me to submit a pull request? If yes, can you point me to where in the code you handle this?

@JamieMason
Copy link
Owner

Thanks @TxHawks I'm taking a look now.

@JamieMason
Copy link
Owner

I thought this would be an easy fix but now I've done it (6493a63) I realise the existing overrides property work matches the location npm uses for the same feature (https://github.com/npm/rfcs/blob/main/accepted/0036-overrides.md) even though the work in syncpack was actually intended for pnpm.

We have 2 overrides properties with the same name in different locations for different package managers. I'll need to think a while what to do, it seems like a breaking change might be needed to make "overrides" apply to pnpm, yarn, and npm overrides/resolutions together but there could be negative consequences of that – or add yet another option which applies only to pnpm overrides.

@TxHawks
Copy link
Author

TxHawks commented May 31, 2022

Since it's another package manager a different property location, it makes sense to me that it would be another option. --pnpm-overrides seems right, since it both makes the intention clear and mirrors the actual property structure

@JamieMason
Copy link
Owner

Thanks yeah I'd been leaning the same way.

JamieMason added a commit that referenced this issue May 31, 2022
Closes #78

BREAKING CHANGE:
The `--overrides` option delivered in 6.0.0 was originally intended to
support pnpm, but erroneously read from the `.overrides` property of
package.json files and not `.pnpm.overrides`.

However, npm now also has an `.overrides` property to support the same
functionality for users of npm.

From this release, the `--overrides` option of syncpack now refers to
npm overrides. Pnpm users should change to using the new
`--pnpmOverrides` option instead.
@JamieMason
Copy link
Owner

Released in 8.0.0, let me know if you have any problems

@TxHawks
Copy link
Author

TxHawks commented May 31, 2022

Thanks!

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

No branches or pull requests

2 participants