Closed Bug 1249088 Opened 5 years ago Closed 5 years ago
Add eslint rules for React
We can use the eslint-react-plugin. This issue will be mostly about which rules to use.
Assignee: nobody → lclark
Status: NEW → ASSIGNED
The attached patch adds some basic rules. Here are some I think we'll need to discuss further: - no-multi-comp: doesn't allow multiple component definitions in a single file - sort-comp: what order methods are declared in on the component - sort-prop-types: whether to alphabetize the prop types or not (I would suggest no on this one) If we end up adding a build step for JSX, we should implement the following: - no-unknown-property - react-in-jsx-scope - self-closing-comp - wrap-multilines - and all of the JSX specific rules
Patrick, would you mind giving this a quick review from an ESLint perspective?
Comment on attachment 8723376 [details] [diff] [review] Bug1249088.patch Review of attachment 8723376 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me. I don't know the React eslint plugin, but I see that its rules first check if the code they're reviewing actually contain react components, so it should be fine to have them defined in the top level devtools .eslintrc config file (no side-effects on non react code). Potentially, there could be one drawback: running eslint on the whole devtools codebase might be slower because all files will go through the react rules, even when they don't contain react components. Right now I don't think adding the plugin will make any difference because so much of our code is disabled (so eslint runs in ~5 seconds for me), but running it with --no-ignore takes ~1 minute for me vs ~1 minute 5 seconds with the react plugin. So, not a big deal. If it was to become a problem, it would be easy to move the plugin and rules definitions in a new config file like /devtools/.eslintrc.react and then have each and every folder that contains components define their own .eslintrc that inherit from this one. Don't do this now though, it's unnecessarily complicated and doesn't solve a problem. Worth noting though: with this patch applied, there are many warnings generated by the code in /devtools/client/responsive.html and /devtools/client/aboutdebugging (71 to be precise). My suggestion for this is that they should be errors, and we should fix them in this patch. I don't like eslint warnings because they don't fail our CI environment and so no one really looks at them. If we want to have a rule for something, we might as well make it log errors.
Attachment #8723376 - Flags: review?(pbrosset) → review+
Thanks for thinking through all of this. It's very helpful information. I didn't realize that warnings didn't fail CI. I agree that we should bump them to errors and fix the existing ones. I can do that in a follow up issue.
Updated reviewer in commit message and created follow-up (Bug 1251271)
has problems to apply: applying Bug1249088.patch patching file devtools/.eslintrc Hunk #2 FAILED at 27 1 out of 2 hunks FAILED -- saving rejects to file devtools/.eslintrc.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh Bug1249088.patch
This is my fault, I just landed changes to .eslintrc in the meantime. I'll rebase and lad this one.
You need to log in before you can comment on or make changes to this bug.