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

feat: allow @typescript-eslint/utils@7 #1567

Merged

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Apr 26, 2024

Fixes #1566.

Upgrades the devDependency versions of @typescript-eslint packages to v7 too. Pleasing to see the yarn.lock mostly remain the same but for a removal of a duplicate minimatch version. Does not do this, as the CI matrix had them testing against incompatible ESLint versions, and I'm in a bit of a rush.

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as draft April 26, 2024 13:11
@JoshuaKGoldberg JoshuaKGoldberg force-pushed the typescript-eslint-utils-7 branch from d3c44db to 04f4173 Compare April 26, 2024 13:18
@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review April 26, 2024 13:25
@SimenB
Copy link
Member

SimenB commented Apr 26, 2024

Can we teak the install to also install v7 of the utils?

yarn add --dev eslint@${{ matrix.eslint-version }} @typescript-eslint/eslint-plugin@${{ matrix.ts-eslint-plugin-version }} @typescript-eslint/parser@${{ matrix.ts-eslint-plugin-version }}

Might make sense as its own matrix entirely 🤔

@SimenB SimenB requested a review from G-Rath April 26, 2024 13:41
@G-Rath
Copy link
Collaborator

G-Rath commented Apr 26, 2024

Personally I'm happy to ship this since I use latest Node already, but how sure you are that this will actually result in the right behaviour across Node versions and all package managers? While I'd love if it just works, I've only ever seen this be used for peer dependencies and we're had previous comments from users blaming this plugin for pulling in old versions when really it's because of the package managers.

Copy link
Contributor Author

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):

Personally I'm happy to ship this since I use latest Node already, but how sure you are that this will actually result in the right behaviour across Node versions and all package managers? While I'd love if it just works, I've only ever seen this be used for peer dependencies and we're had previous comments from users blaming this plugin for pulling in old versions when really it's because of the package managers.

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?

Copy link
Collaborator

@G-Rath G-Rath Apr 26, 2024

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 😅)

Copy link
Contributor Author

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!

Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's find out what happens 🤷

@G-Rath G-Rath merged commit 1476f10 into jest-community:main May 3, 2024
34 checks passed
github-actions bot pushed a commit that referenced this pull request May 3, 2024
# [28.5.0](v28.4.0...v28.5.0) (2024-05-03)

### Features

* allow `@typescript-eslint/utils` v7 as a direct dependency ([#1567](#1567)) ([1476f10](1476f10))
Copy link

github-actions bot commented May 3, 2024

🎉 This PR is included in version 28.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@JoshuaKGoldberg JoshuaKGoldberg deleted the typescript-eslint-utils-7 branch May 4, 2024 00:21
@SimenB
Copy link
Member

SimenB commented May 13, 2024

Note that this does indeed pull in v7, and without manually messing in the lockfile, there's no way to get v6. So our minimum supported node version lies out of the box

@G-Rath
Copy link
Collaborator

G-Rath commented May 13, 2024

I wouldn't say it's an outright lie - that's arguably a package manager bug since all our constraints and versions are valid but I expect this'll be a hard one to get fixed on their end (as it'll be considered "too hard")

I did have a play with it though after publishing and I thought npm at least did do the right thing - I'll double check tomorrow morning

@SimenB
Copy link
Member

SimenB commented May 13, 2024

Missing feature rather than a bug, I'd say 😀 None of them say they prioritze the least amount of deps or choosing a dep based on engines

@rickhanlonii
Copy link
Contributor

Fwiw this broke the react repo from upgrading to 28.5, so I had to pin to 28.4, but only happens in CI where we're probably using a different node version there, still annoying to happen in a minor.

@SimenB
Copy link
Member

SimenB commented Jun 4, 2024

😢

But ok, package managers don't deal with this in a desirable way (meaning in practice it's a breaking change), so we should revert

@nklowns
Copy link

nklowns commented Jul 5, 2024

Note that this does indeed pull in v7, and without manually messing in the lockfile, there's no way to get v6. So our minimum supported node version lies out of the box

This can be circumvented by using resolutions, setting the @^6 version for the package, this is how I used to run the project compatible with node@16

Resolutions Documentation on yarn

{
    ...
    "resolutions": {
        "@typescript-eslint/utils": "^6",
    },
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow @typescript-eslint/utils@7 as a dependency
5 participants