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: upgrade @typescript-eslint/utils to v6 #1508

Merged
merged 11 commits into from
Mar 22, 2024
Merged

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Feb 16, 2024

Really the changes needed were just type and testing based, though I'm not sure if we can support v5 without a breaking change anyway because we'd have to switch to requiring it as a peer dependency which is arguably still breaking?

v6 dropped support for Node v14 which we still support.

Closes #1401

(this is meant to be the min. required for the upgrade - I'll do other stuff like updating the linting plugins in another PR)

@SimenB
Copy link
Member

SimenB commented Mar 21, 2024

rebase and it should be good?

@G-Rath
Copy link
Collaborator Author

G-Rath commented Mar 21, 2024

Yup, I'm just doing a couple of things at once while I wait for CIs in different projects to finish 😛

@@ -95,7 +95,7 @@
]
},
"dependencies": {
"@typescript-eslint/utils": "^5.10.0"
"@typescript-eslint/utils": "^6.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

image

as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we're still supporting @typescript-eslint/eslint-plugin v5 and don't use a lot of the actual rules so I don't think there's a strong advantage - I'll double check that but I don't think it should block landing this right now

Copy link
Member

Choose a reason for hiding this comment

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

I think we can drop it - no reason too keep support when bumping version (when we're landing as major regardless)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok but can we do that in a PR after this so it gets a dedicated changelog entry?

Copy link
Member

@SimenB SimenB Mar 21, 2024

Choose a reason for hiding this comment

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

i.e. I think it makes sense to add support for further majors as a minor, but if we're doing a major release regardless, I don't think it's super valuable (beyond this special case when we're 2 majors behind 😅 )

Copy link
Member

Choose a reason for hiding this comment

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

for sure!

Copy link
Member

Choose a reason for hiding this comment

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

(sorry, I had the tab open and GH isn't super good with live updates 😅 let's blame rails!)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: update this to the last v6 version.
That way you're enforcing that package managers always give you the latest version, rather than trying to resolve to some random one with fewer bug fixes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tend to favor not doing that to enable better deduplication downstream, given we consider this a very stable dependency

'Promise.resolve().then(console.log);',
"['1', '2', '3'].map(Number.parseInt);",
'[5.2, 7.1, 3.6].map(Math.floor);',
'const x = console.log;',
...(parseInt(rawTypeScriptESLintPluginVersion.split('.')[0], 10) >= 6
Copy link
Member

Choose a reason for hiding this comment

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

won't this always be true?

Copy link
Member

Choose a reason for hiding this comment

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

(also, we should use semver module for this)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No because we still support and test against @typescript-eslint/eslint-plugin v5

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll not replace this with semver since it'll just go away right after this PR when we drop support for v5

@G-Rath G-Rath requested a review from SimenB March 21, 2024 20:39
@@ -70,11 +68,14 @@ export default createRule<
ignore: {
type: 'array',
items: {
type: 'string',
// for some reason TypeScript thinks this _must_ be a read-only
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably need to update our parameter types to be const parameters to avoid weirdness like this.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

👍

@G-Rath G-Rath merged commit dc6e8cd into next Mar 22, 2024
38 checks passed
@G-Rath G-Rath deleted the update-typescript-eslint branch March 22, 2024 19:32
github-actions bot pushed a commit that referenced this pull request Mar 22, 2024
# [28.0.0-next.3](v28.0.0-next.2...v28.0.0-next.3) (2024-03-22)

### Features

* upgrade `@typescript-eslint/utils` to v6 ([#1508](#1508)) ([dc6e8cd](dc6e8cd))
Copy link

🎉 This PR is included in version 28.0.0-next.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions bot pushed a commit that referenced this pull request Apr 6, 2024
# [28.0.0](v27.9.0...v28.0.0) (2024-04-06)

### Bug Fixes

* allow ESLint 9 as peer dependency ([#1547](#1547)) ([3c5e167](3c5e167))
* drop support for Node 19 ([#1548](#1548)) ([c87e388](c87e388))
* **no-large-snapshots:** avoid `instanceof RegExp` check for ESLint v9 compatibility ([#1542](#1542)) ([af4a9c9](af4a9c9))

### Features

* drop support for `@typescript-eslint/eslint-plugin` v5 ([#1530](#1530)) ([150e355](150e355))
* drop support for Node v14 ([#1527](#1527)) ([df5e580](df5e580))
* remove `no-if` rule ([#1528](#1528)) ([f976fc8](f976fc8))
* remove `snapshot` processor and `flat/snapshot` config ([#1532](#1532)) ([98087f9](98087f9))
* upgrade `@typescript-eslint/utils` to v6 ([#1508](#1508)) ([dc6e8cd](dc6e8cd))

### BREAKING CHANGES

* Node v19 is no longer supported
* removed unneeded `snapshot` processor and `flat/snapshot` config
* dropped support for `@typescript-eslint/eslint-plugin` v5
* dropped support for Node v14
* removed `no-if` in favor of `no-conditional-in-test`
Copy link

github-actions bot commented Apr 6, 2024

🎉 This PR is included in version 28.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

4 participants