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

npm: Add support for node compatibility checks #4826

Closed
sidharthachatterjee opened this issue Nov 18, 2019 · 31 comments · Fixed by #22447
Closed

npm: Add support for node compatibility checks #4826

sidharthachatterjee opened this issue Nov 18, 2019 · 31 comments · Fixed by #22447
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) v36

Comments

@sidharthachatterjee
Copy link

What would you like Renovate to be able to do?
It would be very nice if Renovate supported some more compatibility checks (beyond semver versions) like the engine field in a package's manifest for instance. An example scenario follows:

Changing the engine key in package.json from 6 to >= 8 for example is a breaking change (for us) and we've seen some packages in the wild do it in patch updates. Currently Renovate would (rightly) open a PR including this update.

It would be nice if it was possible to skip this update by virtue of the engine key no longer being compatible.

Describe the solution you'd like
Check the engine key in a package's manifest and use that to test compatibility.

Describe alternatives you've considered
Hope and pray that packages in the wild respect semver? 🤷‍♂

Additional context
None

@rarkins
Copy link
Collaborator

rarkins commented Nov 18, 2019

Can you locate any packages that have done it? We need to study the npm metadata to see what we can work with.

@wardpeet
Copy link

Thanks @sidharthachatterjee. It's not just patch versions. It also helps us with major deps as we have to go through each package.json to see if we are compatible.

Having them not in the list of the renovate bot issue would be amazing or maybe somewhere at the bottom.

@sidharthachatterjee
Copy link
Author

@wardpeet Remember any example case of a package doing this?

@rarkins
Copy link
Collaborator

rarkins commented Nov 18, 2019

Generally with compatibility we completely suppress incompatible versions. So this would mean that you wouldn't even see the updates/PRs until one day in the future where you update your engines or config somehow. Is that what you're after?

@sidharthachatterjee
Copy link
Author

@rarkins Yup, while it would be nice to see them in the master issue, completely suppressing them should be fine too

@rarkins
Copy link
Collaborator

rarkins commented Nov 19, 2019

BTW even a major update should be fine to test against, just need some examples

@wardpeet
Copy link

wardpeet commented Nov 19, 2019

On this issue gatsbyjs/gatsby#16840
You can find the following incompatible versions:

  • fix: update dependency css-loader to v3
  • fix: update dependency file-loader to v4
  • fix: update dependency got to v9
  • fix: update dependency style-loader to v1
  • chore: update dependency stylelint to v12

There are probably some more but this should get you going ^^

Our engine is set to node 8.0.0

@rarkins
Copy link
Collaborator

rarkins commented Nov 19, 2019

On master branch package.json I see "node": ">=6.11.5". Isn’t that what we should be looking at?

@wardpeet
Copy link

Seems like we never changed the root package.json. For dependencies, it should check the engine where it is defined in. For devDependencies it should do the same unless it's in a monorepo than it should look at the root one.

This is the idea I had, unsure if it make sense so feel free to tweak it.

@rarkins
Copy link
Collaborator

rarkins commented Nov 19, 2019

Logic-wise I think we need to exclude any package that has a minimum version higher than your minimum? Even if by a patch? And if you have updated to an “incompatible” dependency already it would mean we ignore all upgrades for it or suggest to downgrade it? Lots of edge cases..

@wardpeet
Copy link

wardpeet commented Nov 19, 2019

Logic-wise I think we need to exclude any package that has a minimum version higher than your minimum?

Yes 👍

And if you have updated to an “incompatible” dependency already it would mean we ignore all upgrades for it or suggest to downgrade it? Lots of edge cases..

I don't think it's necessary to downgrade to get this shipped. It might be a nice follow-up.

@rarkins rarkins added manager:npm package.json files (npm/yarn/pnpm) type:feature Feature (new functionality) needs-requirements priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others labels Dec 11, 2019
@rarkins rarkins changed the title Add support for compatibility checks npm: Add support for node compatibility checks Dec 11, 2019
@ljharb
Copy link

ljharb commented Oct 25, 2020

The way I think this should work is that if a project has declared "engines.node", and it's not set to "*", and the dependency being added declares an "engines.node" range that is not a superset of the project's range (ie, if the project is ">= 8" and the dep is ">= 10"), that it should be (configurably) skipped, or notified, but not suggested as an upgrade via a PR.

https://npmjs.com/ls-engines may help with this - iow, the ideal output before the dep should be the same output after the dep.

@rarkins
Copy link
Collaborator

rarkins commented Oct 27, 2020

I like the idea of simply checking that the candidate version's node constraint produces a subset of the repo's node constraint, and skip the candidate version if so. We could try calculating that "algebraically" but it's perhaps simpler and more foolproof to simply fetch the list of known node versions once and then filter and compare.

@rarkins
Copy link
Collaborator

rarkins commented Oct 27, 2020

Next step: a simple "reproduction" repo that today generates PRs for incompatible node versions.

@ljharb
Copy link

ljharb commented Oct 27, 2020

Combining the list of node versions from https://nodejs.org/dist/index.tab with the semver package should you calculate it relatively trivially.

@ljharb
Copy link

ljharb commented Mar 24, 2021

Sounds great to me.

I'm pretty sure npm's own algorithm is usable in https://npmjs.com/@npmcli/arborist, but you'd have to dig to figure out exactly which.

@rarkins rarkins added auto:reproduction A minimal reproduction is necessary to proceed status:requirements Full requirements are not yet known, so implementation should not be started and removed status:ready labels Jan 14, 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 May 11, 2023

Can anyone put together a reproduction repo? An example could be:

  • Current repo's engines say Node 16 or 18
  • Latest version of a dependency supports Node 18 only

@remal
Copy link

remal commented May 26, 2023

@rarkins, take a look: remal-github-actions/template-typescript#687

@octokit/plugin-throttling v6 supports Node >= 18. However, my .nvmrc says that Node v16 should be used.

I don't want @octokit/plugin-throttling to be updated to v6 until I upgrade Node.js.

Renovate does see that Node.js constraint is v16:

DEBUG: Using node constraint "16" from .nvmrc

@rarkins
Copy link
Collaborator

rarkins commented May 27, 2023

@remal this feature would need to be based on the engines constraints and not .nvmrc if it is to be accurate. In your case you have >= 16 in engines: https://github.com/remal-github-actions/template-typescript/blob/248061d6e62fc960d0c5afd57f178bb62cad688e/package.json#L5-L7

I prototyped the feature using https://github.com/renovate-reproductions/4826 but already we see in this feature that it gets complicated. A number of dependencies you are using do support Node 16 but only the LTS range, e.g. ^16.10.0. This means that technically they do not support your engines constraints and need to be rolled back if strict mode was on. Example, ts-jest supports "^14.15.0 || ^16.10.0 || >=18.0.0"

Even with that resolved, the next problem is that your declaration of >= 16 means that v17 is supported too. A library like ts-jest doesn't support v17, so therefore isn't compatible.

I reduced the noise by doing strict filtering only for dependencies (i.e. not devDependencies) and all rollback PRs were closed: https://github.com/renovate-reproductions/4826/pulls?q=is%3Apr+sort%3Aupdated-desc+is%3Aclosed

But I still think this feature needs further brainstorming before it's ready.

@remal
Copy link

remal commented May 27, 2023

I looked at Removate sources today and found that it looks at .nvmrc first, then some other file (don't remember), and only then - package.json.

I left >=16 in package.json, because I don't want to limit the local environment. However, my CI/CD jobs are strictly limited to Node.js v16 (GitHub actions don't support Node 18 runtime at the moment).

What I'd like Renovate to do is not to update dependencies to versions that require Node v17, 18, etc. Is it possible? If yes, the how? Do you have any other suggestions for my case?

rarkins added a commit that referenced this issue May 27, 2023
rarkins added a commit that referenced this issue May 27, 2023
@rarkins
Copy link
Collaborator

rarkins commented May 27, 2023

I looked at Removate sources today and found that it looks at .nvmrc first, then some other file (don't remember), and only then - package.json.

You're mixing up two related concepts:

  • What you're seeing there is "how should Renovate decide which version of Node.js to install before running npm install to update lockfiles?"
  • What we're talking about here is "how should Renovate decide if dependency versions are compatible with your library?"

In the first case Renovate needs to pick one version - so using .nvmrc makes most sense. In the second case it needs to make sure that the package supports all versions you need - not just one.

I left >=16 in package.json, because I don't want to limit the local environment. However, my CI/CD jobs are strictly limited to Node.js v16 (GitHub actions don't support Node 18 runtime at the moment).

This is up to you, but it will influence the lookup logic

What I'd like Renovate to do is not to update dependencies to versions that require Node v17, 18, etc. Is it possible? If yes, the how? Do you have any other suggestions for my case?

First of all, this is not possible today and that's why this issue is open.

But once it's implemented, then either you declare your engines as narrower, such as ^16.10.0, or you would manually configure config.constraints to override your declared engines.

@rarkins
Copy link
Collaborator

rarkins commented May 27, 2023

PR now open: #22447

@rarkins rarkins added status:in-progress Someone is working on implementation and removed status:requirements Full requirements are not yet known, so implementation should not be started labels May 27, 2023
@rarkins rarkins self-assigned this May 27, 2023
@remal
Copy link

remal commented Jun 26, 2023

@rarkins, as I can see, the PR is merged. But I still couldn't make it work for this repo: https://github.com/remal-github-actions/template-typescript. Should it work? Is it still in progress? Am I doing something wrong?

@rarkins
Copy link
Collaborator

rarkins commented Jun 26, 2023

It's part of the v36 branch so not part of an official release yet

@rarkins rarkins added v36 and removed auto:reproduction A minimal reproduction is necessary to proceed labels Jun 27, 2023
rarkins added a commit that referenced this issue Jul 4, 2023
Closes #4826

Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
rarkins added a commit that referenced this issue Jul 4, 2023
Closes #4826

Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
@rarkins rarkins closed this as completed in ad0479a Jul 4, 2023
@renovate-release
Copy link
Collaborator

🎉 This issue has been resolved in version 36.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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) v36
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants