-
Notifications
You must be signed in to change notification settings - Fork 55
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
update eslint versions #358
Conversation
fix test changeset
0ce2c52
to
5dd3fa2
Compare
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.
Thanks for the updates.
A few tidying tasks that should be done prior to merge:
- Deduplicate the yarn.lock file with
npx yarn-dedupe
- What required the eslint peerDependency bump? Can it be lowered?
- Please write a more complete changeset entry. This is a breaking change as it removes rules - those need to be documented along with the update type being marked as
major
"jsx-ast-utils": "^3.2.1", | ||
"pkg-dir": "^5.0.0", | ||
"pluralize": "^8.0.0" | ||
}, | ||
"peerDependencies": { | ||
"eslint": "^8.3.0" | ||
"eslint": "^8.32.0" |
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.
Changing the peer dependency on eslint is a breaking change. What package causes this bump and can it be reverted so that we don't force consumers to upgrade more than required?
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.
It needs to be^8.15.0
. Happy to match that requirement if you judge is better 👍
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'm not sure what file you're linking me to there, the deeplink doesn't go anywhere for me (I guess because some file doesn't want to expand as the diff is so big). Could you clarify?
Searching the typescript-eslint codebase's package.json files for mention of eslint suggests that its peerDependency range for eslint is "^6.0.0 || ^7.0.0 || ^8.0.0"
, which doesn't seem to support the requirement change here.
'@shopify/eslint-plugin': patch | ||
--- | ||
|
||
bump eslint dependencies |
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.
This is a major
breaking change thanks to the removal of the the jest/no-jest-import
rule. Please update the update type.
Please write a more complete changelog entry. Please detail what packages were bumped, and to what version, such as https://github.com/Shopify/web-configs/blob/main/packages/eslint-plugin/CHANGELOG.md#breaking-change. You should also detail any potentially breaking changes - notable the removal of the jest/no-jest-import
rule.
Closing as stale |
Description
This PR bumps the versions of each
eslint
dependencies. The main motivation behind that change is to get@typescript-eslint
to handle typescript's newsatisfies
keyword