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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
May 3, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0684f41
feat: allow @typescript-eslint/utils@7
JoshuaKGoldberg Apr 26, 2024
04f4173
fix: typecheck and drop dependencies down
JoshuaKGoldberg Apr 26, 2024
9960bef
chore: drop back down to one type argument
JoshuaKGoldberg Apr 26, 2024
c1e2f2e
chore: add utils to matrix
JoshuaKGoldberg Apr 26, 2024
8158c98
Merge branch 'main' into typescript-eslint-utils-7
G-Rath May 2, 2024
a568518
ci: properly install different versions of utils package
G-Rath May 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/nodejs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ jobs:
test-node:
name:
# prettier-ignore
Test on Node.js v${{ matrix.node-version }}, eslint v${{ matrix.eslint-version }}, and ts-eslint/plugin v${{ matrix.ts-eslint-plugin-version }}
Test on Node.js v${{ matrix.node-version }}, eslint v${{ matrix.eslint-version }}, and ts-eslint v${{ matrix.ts-eslint-plugin-version }}
needs: prepare-yarn-cache-ubuntu
strategy:
fail-fast: false
Expand Down Expand Up @@ -100,6 +100,7 @@ jobs:
install with eslint v${{ matrix.eslint-version }} and ts-eslint/plugin v${{ matrix.ts-eslint-plugin-version }}
run: |
yarn
yarn add @typescript-eslint/utils@${{ matrix.ts-eslint-plugin-version }}
yarn add --dev eslint@${{ matrix.eslint-version }} @typescript-eslint/eslint-plugin@${{ matrix.ts-eslint-plugin-version }} @typescript-eslint/parser@${{ matrix.ts-eslint-plugin-version }}
- name: run tests
# only collect coverage on eslint versions that support dynamic import
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
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 •
edited

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!

Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@
]
},
"dependencies": {
"@typescript-eslint/utils": "^6.0.0"
"@typescript-eslint/utils": "^6.0.0 || ^7.0.0"
},
"devDependencies": {
"@babel/cli": "^7.4.4",
Expand All @@ -83,6 +83,7 @@
"@types/node": "^14.18.26",
"@typescript-eslint/eslint-plugin": "^6.0.0",
"@typescript-eslint/parser": "^6.0.0",
"@typescript-eslint/utils": "^6.0.0",
"babel-jest": "^29.0.0",
"babel-plugin-replace-ts-export-assignment": "^0.0.2",
"dedent": "^1.5.0",
Expand Down