-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
chore: upgrade eslint and relevant dependencies #13891
Conversation
varsIgnorePattern: '^_', | ||
args: 'after-used', | ||
argsIgnorePattern: '^_', | ||
caughtErrors: 'none', |
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.
caughtErrors: 'none',
is required by the new version for unused catch (error)
in the codebase
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.
why would we catch error but not use it? Maybe we should adhere to default here?
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.
If we decided to do so, I want to change the rule separately, wanted to keep this PR only the migration scope.
allowShortCircuit: true, | ||
allowTernary: true, |
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.
Allow code patterns used in the codebase, such as
isBrowser() && doBrowserStuff()
runA ? a() : b()
Question: should we disallow this, as the suggestion makes sense.
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 do feel this allows for easy shorthand. I feel nothing wrong in doing this 😅
|
||
'@typescript-eslint/no-useless-constructor': 'error', | ||
'@typescript-eslint/no-require-imports': 'off', | ||
|
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.
New version disallows using require()
by default, we are using it in test codebase for module loading and mocking, as well as in the react-native
for module resolution at runtime.
Description of changes
Upgrade eslint to the latest version, v8 is EOL on 10/05.
eslint.config.mjs
is generated by eslint provided migration toolnpx @eslint/migrate-config .eslintrc.js
with some necessary manual fixes for backwards compatibility@typescript/eslint-plugin
rule deprecations)Issue #, if available
Description of how you validated changes
yarn build && yarn lint && yarn test
Checklist
yarn test
passesChecklist for repo maintainers
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.