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

Support for npm 8.3 Overrides #15278

Closed
karlhorky opened this issue Apr 23, 2022 · 21 comments · Fixed by #15351
Closed

Support for npm 8.3 Overrides #15278

karlhorky opened this issue Apr 23, 2022 · 21 comments · Fixed by #15351
Assignees
Labels
manager:npm package.json files (npm/yarn/pnpm) priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:in-progress Someone is working on implementation type:feature Feature (new functionality)

Comments

@karlhorky
Copy link
Contributor

karlhorky commented Apr 23, 2022

What would you like Renovate to be able to do?

Hi there, first of all, thanks for your continued effort on Renovate! It's such an amazing tool, so valuable.

Similar to #1318 (updating Yarn Resolutions in package.json), it would be great if Renovate also supported npm Overrides (introduced in npm 8.3) in the same way - eg. when the version in package.json under the "overrides" object matches the version of the dependency in dependencies or devDependencies.

If you have any ideas on how this should be implemented, please tell us here.

I'm assuming that the MVP of this could be the same feature as implemented in 163ce43 (based on the latest logic in lib/modules/manager/npm/update/dependency/index.ts), reusing much of the code

However, I also see that there is a depType which may be set to resolutions, so this may also be a consideration for this change (should there be a new depType called overrides?):

if (match && depType === 'resolutions') {
const patch = oldValue.replace(match[0], `${match[1]}${newValue}#`);
parsedContents[depType]![depName] = patch;
newString = `"${patch}"`;
}

Is this a feature you are interested in implementing yourself?

No


Workaround

For now, I'm just duplicating the "overrides" data to "resolutions" and relying on the Yarn Resolutions feature of Renovate bot. In order to automatically update "overrides", I wrote this GitHub Actions workflow: https://github.com/upleveled/changes-codealong/blob/main/.github/workflows/copy-resolutions-to-overrides.yml

See example PR here: upleveled/changes-codealong#62

@karlhorky karlhorky added priority-5-triage status:requirements Full requirements are not yet known, so implementation should not be started type:feature Feature (new functionality) labels Apr 23, 2022
karlhorky added a commit to upleveled/changes-codealong that referenced this issue Apr 23, 2022
@rarkins rarkins added manager:npm package.json files (npm/yarn/pnpm) auto:reproduction A minimal reproduction is necessary to proceed labels Apr 23, 2022
@github-actions
Copy link
Contributor

Hi there,

Help us by making a minimal reproduction repository.

Before we can start work on your issue we first need to know exactly what's causing the current behavior. A minimal reproduction helps us with this.

To get started, please read our guide on creating a minimal reproduction to understand what is needed.

We may close the issue if you (or someone else) have not provided a minimal reproduction within two weeks. If you need more time, or are stuck, please ask for help or more time in a comment.

Good luck,

The Renovate team

@rarkins
Copy link
Collaborator

rarkins commented Apr 23, 2022

Can you provide a minimal reproduction which demonstrates a "wrong" PR but which should be updating overrides?

@karlhorky
Copy link
Contributor Author

karlhorky commented Apr 23, 2022

upleveled/changes-codealong#62 is a minimal repro PR (also above in the Workaround section) - the first commit shows the "wrong" PR commit (only updating the "resolutions" key) and the second commit is my GitHub Actions workflow that copied the object to the "overrides" key in package.json:

Screen Shot 2022-04-23 at 13 39 31

@rarkins
Copy link
Collaborator

rarkins commented Apr 23, 2022

Please check the above link for what we mean by minimal. The example PR you gave updates 10+ packages, which makes it complex to debug during development

@karlhorky
Copy link
Contributor Author

Ok, here's a reproduction repo PR with a single dep: https://github.com/karlhorky/renovate-repro-npm-overrides/pull/3/files

@rarkins rarkins added reproduction:provided priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others and removed auto:reproduction A minimal reproduction is necessary to proceed priority-5-triage status:requirements Full requirements are not yet known, so implementation should not be started labels Apr 25, 2022
@PhilipAbed
Copy link
Collaborator

"overrides" is supposed to be used for Transitive Dependencies,
but in the Minimal Reproduction Repository u have it in both devDeps and Resolutions too.

"devDependencies": {
   "@types/react": "18.0.6"
 },
 "resolutions": {
   "@types/react": "18.0.6"
 },

this does not look like a real usage.

Anyway, Overrides should be treated the same way that we treat dependencies/devDependencies, as it's an actual transitive dependency that is effectively used.

it's pretty easy to get a reproduction data, anything with overrides will do.

@karlhorky
Copy link
Contributor Author

karlhorky commented Apr 25, 2022

"overrides" is supposed to be used for Transitive Dependencies, but in the Minimal Reproduction Repository u have it in both devDeps and Resolutions too.

this does not look like a real usage.

It's possible that users have a direct dependency on something as well as it being in transitive dependencies. Sometimes it's really important to have the same version (such as with ESLint shared configs + plugins).

So this can indeed be a real-world use case.

@PhilipAbed
Copy link
Collaborator

PhilipAbed commented Apr 25, 2022

if you want something as a direct dependency then you put it in dependencies/devDependencies

If the dependency is in Transitive as well then you add a path to it like this:
image

i guess its also used like you said you are right

@rarkins
Copy link
Collaborator

rarkins commented Apr 25, 2022

I have a feeling that was a convention we used for workspaces to designate that it's a resolution the user wants updated

@karlhorky
Copy link
Contributor Author

used for workspaces

Right, this is another usecase - we are using Yarn Resolutions in a monorepo (with Yarn Workspaces) which may have @types/react as a transitive dep in some part of the tree (but we also need @types/react at the root). Without overriding all versions of this package to be the same version, it can cause some very confusing TS errors.

@hasanwhitesource
Copy link
Contributor

@rarkins Should we create group names for children of packages ?
for example :

{
  "overrides": {
    "bar": {
      "foo": "1.0.0"
    }
  }
}

Should we in this case make foo have a groupName of bar ?

@hppycoder
Copy link

How did you get NPM 8 to run which is part of Node 16? I was under the impression that renovate is at 14.x using Kubernetes install path.

@renovate-release
Copy link
Collaborator

🎉 This issue has been resolved in version 32.60.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@karlhorky
Copy link
Contributor Author

karlhorky commented May 18, 2022

Just tested 32.60.0 and it's working!

It updated the "overrides" key in my package.json in this PR:

https://github.com/upleveled/changes-codealong/pull/85/files

@rarkins
Copy link
Collaborator

rarkins commented May 18, 2022

@karlhorky awesome, thanks for confirming!

@karlhorky
Copy link
Contributor Author

karlhorky commented May 18, 2022

Oh, I tricked myself - it's actually not working - it was my GitHub Actions workflow script 😩

You can see the problem here - first of all Renovate bot updates the dependencies (without "overrides"), which fails the tests and also creates an "Artifact update problem":

Screen Shot 2022-05-18 at 12 44 24

Then, further down, my GitHub Actions workflow script (which I thought I had disabled) comes along and fixes it, which caused the PR to be ok and have the changes to "overrides".

I've now triggered the Renovate bot to open a new PR, which doesn't change the "overrides": upleveled/changes-codealong#85

Screen Shot 2022-05-18 at 12 50 27

@karlhorky
Copy link
Contributor Author

karlhorky commented May 18, 2022

@hasanwhitesource @rarkins is this PR above enough of a reproduction to figure out what's going on?

Happy to retrigger this PR anytime there are bot updates published to check if they work...

@rarkins rarkins reopened this May 18, 2022
@rarkins
Copy link
Collaborator

rarkins commented May 18, 2022

Reopened the issue, @hasanwhitesource can you take a look? Assuming that you tested it on a demo repo, it could be good to share and compare that against @karlhorky's repo

@rarkins rarkins added status:in-progress Someone is working on implementation and removed status:ready labels May 18, 2022
@karlhorky
Copy link
Contributor Author

karlhorky commented May 18, 2022

One interesting side note (not sure if this is relevant) is that there are a bunch of other dependencies showing up in my dependency dashboard now (not being combined with the non-major pull requests that the bots are creating):

Screen Shot 2022-05-18 at 12 55 43

Wonder if this is related at all...

@karlhorky
Copy link
Contributor Author

karlhorky commented May 18, 2022

Oh wait, maybe there was a new dependency type called 'overrides' added?

Ahh, I need to update my shared config to also include this new 'overrides' dependency type....

Edit: Done karlhorky/renovate-config@30d6a6d

@karlhorky
Copy link
Contributor Author

Yes, this was the problem 🤦‍♂️ Sorry for the noise!

It's indeed working, closing this again 👍

Screen Shot 2022-05-18 at 13 03 22

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
manager:npm package.json files (npm/yarn/pnpm) priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:in-progress Someone is working on implementation type:feature Feature (new functionality)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants