Integrate eslint & precommit to codebase#815
Conversation
|
Deploy preview for docusaurus-preview ready! Built with commit 417efda |
endiliey
force-pushed
the
eslint
branch
2 times, most recently
from
6906482 to
1f7df7f
Compare
endiliey
force-pushed
the
eslint
branch
2 times, most recently
from
60dcad8 to
249fc7d
Compare
yangshun
left a comment
yangshun
left a comment
There was a problem hiding this comment.
Looks great! What are the next steps you're planning for the ESLint integration? Are we enabling more rules?
.eslintrc.json
Outdated
| @@ -0,0 +1,16 @@ | |||
| { | |||
There was a problem hiding this comment.
Let's use .js extension. It's much more flexible than JSON and we can add comments.
| "eslint": "^5.0.1", | ||
| "eslint-config-fbjs": "^2.0.1", | ||
| "eslint-config-prettier": "^2.9.0", | ||
| "eslint-plugin-babel": "^5.1.0", |
There was a problem hiding this comment.
Are we using them yet? I don't see them in eslintrc. Maybe we should add them later if we're not using yet.
There was a problem hiding this comment.
eslint-config-fbjs requires it as peer dependencies
https://github.com/facebook/fbjs/blob/d308fa83c99c93e8e588de3396cf55b31e56b14e/packages/eslint-config-fbjs/package.json#L21-L28
I've only added eslint-config-prettier so far to remove eslint-prettier conflict.
Edit: So for now we're effectively using eslint-config-fbjs & eslint-config-prettier
.eslintrc.json
Outdated
| } | ||
| } | ||
| ] | ||
| } No newline at end of file |
.eslintrc.json
Outdated
| @@ -0,0 +1,16 @@ | |||
| { | |||
| "extends": ["fbjs", "prettier"], | |||
There was a problem hiding this comment.
Are we adding more of them later?
There was a problem hiding this comment.
I think not so soon. fbjs config is quite ok for now. We can always overrides their rules later on
| }); | ||
|
|
||
| function afterAll() { | ||
| afterAll(() => { |
There was a problem hiding this comment.
This changes the meaning - It was declaring a function before, now it's executing it. The after version looks more correct though.
There was a problem hiding this comment.
yup. it's actually caught by the linter. Otherwise the function is never used
|
Feel free to merge this when you feel it's ready. We could add more rules later on/fix more issues. |
|
I'm going to add some more specific rules for libs that can cause bugs. Lot of linting errors are turned off at fbjs. |
|
Hmm in that case I wonder if fbjs is the right one to use. Let's get @JoelMarcey's opinion on this when he's available. But in the meanwhile we can merge this. |
|
I'm changing it to |
|
I approve of the |
5 participants
Motivation
P.S Our ESLint config is based on Facebook ESLint configuration (https://github.com/facebook/fbjs/tree/master/packages/eslint-config-fbjs)
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
ESLint
Precommit hook
