-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Comments
Can you locate any packages that have done it? We need to study the npm metadata to see what we can work with. |
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. |
@wardpeet Remember any example case of a package doing this? |
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? |
@rarkins Yup, while it would be nice to see them in the master issue, completely suppressing them should be fine too |
BTW even a major update should be fine to test against, just need some examples |
On this issue gatsbyjs/gatsby#16840
There are probably some more but this should get you going ^^ Our engine is set to node 8.0.0 |
On master branch |
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. |
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.. |
Yes 👍
I don't think it's necessary to downgrade to get this shipped. It might be a nice follow-up. |
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. |
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. |
Next step: a simple "reproduction" repo that today generates PRs for incompatible node versions. |
Combining the list of node versions from https://nodejs.org/dist/index.tab with the |
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. |
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 |
Can anyone put together a reproduction repo? An example could be:
|
@rarkins, take a look: remal-github-actions/template-typescript#687
I don't want Renovate does see that Node.js constraint is v16:
|
@remal this feature would need to be based on the 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. Even with that resolved, the next problem is that your declaration of I reduced the noise by doing strict filtering only for But I still think this feature needs further brainstorming before it's ready. |
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 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? |
You're mixing up two related concepts:
In the first case Renovate needs to pick one version - so using
This is up to you, but it will influence the lookup logic
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 |
PR now open: #22447 |
@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? |
It's part of the v36 branch so not part of an official release yet |
Closes #4826 Co-authored-by: HonkingGoose <[email protected]>
Closes #4826 Co-authored-by: HonkingGoose <[email protected]>
🎉 This issue has been resolved in version 36.0.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
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
The text was updated successfully, but these errors were encountered: