-
Notifications
You must be signed in to change notification settings - Fork 13
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
[Deps] update @npmcli/arborist
#90
Conversation
Update the dependency `@npmcli/arborist` from v6 to v7 in order to transitively upgrade dependencies such that `@npmcli/move-file` is removed from the dependency tree. `@npmcli/move-file` had been deprecated because "This functionality has been moved to @npmcli/fs". The breaking changes from v6 to v7 are only that support for Node.js <=16.13 has been dropped. This means that this upgrade would require a major version bump here as well, since we currently support >=14.17. This change has been included here as well, based on the engines.node value for `@npmcli/arborist`. The effect can be seen from within this repository as follows, before: $ npm i --omit dev npm WARN deprecated @npmcli/move-file@2.0.1: This functionality has been moved to @npmcli/fs added 263 packages, and audited 264 packages in 2s [...] After: $ npm i --omit dev added 213 packages, and audited 214 packages in 2s [...]
Thanks for sending this surprise contribution! Looking into our engines/versions situation here, I have wound up confused again We declare I'm inclined to just remove |
If I'm not mistaken, the reason it only tested on 16, 18, 19, 20, and 21 is because I updated the CI configuration as well. The CI for 4f3985c did run tests against Node 14.
I'll leave that up to you/the maintainers of this project, I think you make a good case for it in #91 though 👍 That being said, you'd still need to decide which version of Node.js to test against. |
The CI config doesn't look at engines at all, and removing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of this update? What benefit do we get? I only see a downside if we're just dropping node 14.
Note that deprecation warnings matter precisely zero; that particular package shouldn't be deprecated and it was wrong for its maintainer to do so.
On testing, I suspect we may have dropped Node.js 14 support already, by not testing on it. I'm on vacation now, but I'll be happily surprised if I get around to testing on Node.js 14, everything installs, and the tests pass. I'm not up-to-date on goings-on with At the same time, I'm generally in favor of following npm's modules closely, and would take deprecation warnings and the like from them seriously. But I'd expect them to be pretty good about "announcing" transition from deprecation to breaking removal via SemVer. |
@kemitchell we are testing on it. It's only this PR that drops tests for it. Here's an example from your "remove engines" PR: https://github.com/jslicense/licensee.js/actions/runs/6684697210/job/18162248337 node 14 still works just fine, and is tested. |
@ljharb thanks so much. I was lost in the hall of mirrors here. While I have you, do you happen to have a link for the |
I mean, they clearly don't think it's a mistake - but imo deprecating |
I hear you. But unless npm itself drops Arborist, or Arborist veers off in a completely unexpected direction, I suspect we'll be riding along with the npm team and saying "thank you" for whatever work they can put out, as bumpy as the going may be. The view of If things become untenable, we can always go back to recursing the directory ourselves. For now I think we just wait and see what happens. I can live with warnings. We've all seen a few. |
Certainly whenever there's a compelling reason to upgrade and do a major bump, we should - I just don't personally think "deprecation warnings" are compelling. |
Just to clarify, the only motivation is indeed to resolve the deprecation warning.
While I personally disagree with that exact statement (won't go into detail without a prompt), I can definitely live with the decision that resolving it is less important than compatibility with older Node.js versions. |
Update the dependency
@npmcli/arborist
from v6 to v7 in order to transitively upgrade dependencies such that@npmcli/move-file
is removed from the (runtime) dependency tree (this follows the recent release ofnode-gyp
v10.0.0). The motivation for this is that@npmcli/move-file
has been deprecated because "This functionality has been moved to@npmcli/fs
" for a while.The breaking change from v6 to v7 is only that support for Node.js <=16.13 has been dropped. The implication is that that this change needs require a major version bump for
licensee
as well, since we currently support >=14.17. This new minimum Node.js version has been included here as well, based on theengines.node
value for@npmcli/arborist
. - See also #85.The effect can be seen from within this repository as follows, before:
after: