We are using several eslint plugins to support our eslint config:
eslint-config-airbnb
eslint-config-prettier
eslint-plugin-import
eslint-plugin-jquery
eslint-plugin-jsx-a11y
eslint-plugin-prettier
eslint-plugin-react
eslint-plugin-react-hooks
Several of them are a dependency of the airbnb config, we should try to simplify our config so that we don't bring in unnecessary react-related packages, and possibly not depend on the airbnb config.
Use eslint-config-airbnb-base which is essentially same as eslint-config-airbnb without React specific plugins and rules.
Make the diff between the current config and eslint:recommended
Agree on what is important/what is not, what to keep/remove
update the config (with minimal changes to existing code hopefully)
JavaScript linting now uses eslint-config-airbnb-base instead of eslint-config-airbnb for linting core JavaScript. Anyone who uses core's ESLint config to lint React or JSX code should add eslint-config-airbnb back to their yarn dev dependencies.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 3239838-12.patch | 21.2 KB | spokje |
| #12 | raw_diff_5-12.txt | 7.43 KB | spokje |
| #11 | 3239838-11.patch | 21.2 KB | spokje |
| #5 | airbnb-vs-base.diff | 18.22 KB | longwave |
| #4 | airbnb-recommended.diff | 60.91 KB | longwave |
Comments
Comment #2
nod_Comment #4
longwaveThis command prints the full config:
The attached diff shows the config difference between the existing airbnb config and the config with eslint:recommended instead.
We don't need any of the react or jsx rules but I don't know what a lot of the "no-X" rules are, and there are quite a lot of differences there.
Comment #5
longwaveJust discovered the existence of https://www.npmjs.com/package/eslint-config-airbnb-base which removes the React and JSX dependencies, but keeps all the other rules. .diff file attached here is an ESLint config diff.
Comment #6
nod_Very nice, thanks for tracking this down :)
Comment #7
nod_no regressions I can see on my end, RTBC
Comment #8
lauriiiUpdated proposed resolution to the issue summary. I'm debating with myself if we need a CR for this 🤔 This would only impact folks who are using React and running our eslint against it (which I guess could be some people). Maybe that's worth mentioning in a CR? I think it's fine that we don't provide that capability. I think we could recommend people doing that to just directly depend on
eslint-config-airbnb.Comment #9
nod_I wouldn't bother, if they use react they probably don't use our toolset anyway. We do provide lint to contrib through drupalci but everyone doing react is on github.
Comment #10
xjmNo CR is OK here. It does need a small release note, though, as a coding standards update.
Comment #11
spokjeReroll of patch #5
Comment #12
spokjeRight,
9.4.x-dev, not10.0.x-dev...Comment #13
longwaveReroll looks good to me, added a release note snippet.
Comment #14
xjmI think we'd also need a 10.0.x version of the patch?
Comment #15
xjmComment #16
spokjePatch in #11 was/is against
10.0.x-devComment #17
xjmAh sorry, got confused by the comments and test results. Restoring the status in the hope that it passes. :)
Comment #19
nod_DB issue in the test so…
Comment #20
spokjeThanks @nod_, saw the failure, triggered a retest, but you've beat me to putting it back on RTBC after the retest passed :)
Comment #22
lauriiiCommitted 3e09146 and pushed to 10.0.x. Also committed the 9.4.x. Thanks!
Comment #24
xjm