chore: Upgrade Airbnb ESLint config versions #331
Conversation
BREAKING CHANGE: This probably breaks something.
| }, | ||
| "peerDependencies": { | ||
| "eslint-plugin-import": "^2.18.2", | ||
| "eslint-plugin-jsx-a11y": "^6.2.3", | ||
| "eslint-plugin-react": "^7.14.3" | ||
| "eslint-plugin-react": "^7.14.3", | ||
| "eslint-plugin-react-hooks": "^1.7.0" |
There was a problem hiding this comment.
should have been a peer dep all along
| @@ -1,16 +1,6 @@ | |||
| module.exports = { | |||
| // We put all styling imports at the end. | |||
| 'import/first': 'off', | |||
| // Allow <Link to>. | |||
| // TODO: Remove this when airbnb includes this in next release. | |||
| 'jsx-a11y/anchor-is-valid': [ | |||
| @@ -21,9 +11,6 @@ module.exports = { | |||
| depth: 25, | |||
| }, | |||
| ], | |||
| 'jsx-a11y/label-has-for': 'off', | |||
| @@ -21,9 +11,6 @@ module.exports = { | |||
| depth: 25, | |||
| }, | |||
| ], | |||
| 'jsx-a11y/label-has-for': 'off', | |||
| 'react-hooks/rules-of-hooks': 'error', | |||
| @@ -21,9 +11,6 @@ module.exports = { | |||
| depth: 25, | |||
| }, | |||
| ], | |||
| 'jsx-a11y/label-has-for': 'off', | |||
| 'react-hooks/rules-of-hooks': 'error', | |||
| 'react-hooks/exhaustive-deps': 'warn', | |||
There was a problem hiding this comment.
this is an error upstream, but we don't allow errors in our pre-commit hooks mostly, so who cares
There was a problem hiding this comment.
only frontend does the no warnings thing, which isn't tenable for telemed right now, it's got a lot of warning that are intentionally left there (e.g. alerts, console.logs)
There was a problem hiding this comment.
okay, rather, it gets auto-fixed, so we still need to forcibly pragma it
| @@ -61,6 +50,7 @@ module.exports = { | |||
| 'getters', | |||
| 'setters', | |||
| '/^(get|set)(?!(InitialState$|DefaultProps$|ChildContext$)).+$/', | |||
| 'instance-methods', | |||
There was a problem hiding this comment.
does this make you need to group arrow functions and methods? That was one of the motivating reasons to override this.
There was a problem hiding this comment.
i thought we overrode this to insert the thing for type definitions
| @@ -23,7 +21,6 @@ module.exports = { | |||
| }, | |||
| ], | |||
| 'no-plusplus': 'off', | |||
| 'no-restricted-globals': ['error'].concat(restirctedGlobals), | |||
There was a problem hiding this comment.
AFAICT this was just to allow isNaN and isFinite? But they both legitimately are sort of crappy.
There was a problem hiding this comment.
not sure i understand, this prevents you from using sketchy globals like name and event things that are likely local identifiers and you'd not get a "undef" warning for if you forgot to actually declare them in code
There was a problem hiding this comment.
no, i mean, this was already defined upstream: https://github.com/airbnb/javascript/blob/21b65e943c361f91af5a484460fb2c8d02e2ff92/packages/eslint-config-airbnb-base/rules/variables.js#L19
There was a problem hiding this comment.
o nice, 👍 didn't know that, just duplicated work then
|
Did we explicitly decide against |
There was a problem hiding this comment.
@jquense On those two TS rules, I ask because those are the things in the (new) recommended config that are most conspicuously missing here. If we don't explicitly want to avoid them, then we should consider extending the recommended TS ESLint config.
|
@jquense i changed the TS config to extend and override the base rule. let me know if this is gtg |
|
lgtm if the parser setup is the same i'm guessing in the base configs ? |
|
i force reverted this because i don't want to major-bump everything. gah |
BREAKING CHANGE: This probably breaks something.