Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@G-Rath threading #1567 (comment):
If it helps, the v7 upgrade didn't include any runtime breaking changes - just minimum version bumps. https://typescript-eslint.io/blog/announcing-typescript-eslint-v7
Personally I don't see a lot of danger in adding the
|| ^7.0.0
here. If a user is still stuck on@typescript-eslint/utils@6
, then they can remain on it. Or in other words, if they're on an old framework or platform version that can't use v7, they can still opt into v6.Maybe I'm not understanding the potential for badness?
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.
I wouldn't call it badness per say - what I'm wondering is if I attempt to install our plugin with this on Node v16, will the package manager correctly determine it should install v6, or will it explode saying "hey I can't install v7 because it requires min. of Node v18"?
In an ideal world the package manager should do the former and I wouldn't be surprised if at least some of them do, but I also wouldn't be surprised if some of them don't - I wouldn't be asking either if this was a pattern I'd seen a lot in the wild but I can't recall ever seeing a package do this for a direct dependency and one of the reasons I'm suspect is I've had to deal with a lot of security advisory hoop jumping because dep a couldn't update to new version of b as they dropped a Node version which if you can "just"
||
in direct dependencies I would expect some of those could have been addressed more easily (maybe combined with some<if new version function exists> || <otherwise use old version function>
kind of logic).This is an example of the kind of thing I have in mind - it's not quite the same, but its an example of a package manager raising a error for a situation it should technically be able to handle (and so at least ideally explain that to the user so they can choose to action it).
As I've said, I personally am happy to ship this - even if we end up reverting it later, hey at least we'll have learnt it does/doesn't work; but I'm just wondering if you know already if all the major package managers will handle this gracefully (or even of just other packages that are doing this for direct dependencies)
(I have tried to test this locally, but it's too faffy for me right now 😅)
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.
Gotcha - no, I haven't looked into this. It's a good question. I'd hope they would do something friendly here!