Closed Bug 1249088 Opened 4 years ago Closed 4 years ago

Add eslint rules for React

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: linclark, Assigned: linclark)

References

Details

Attachments

(1 file, 2 obsolete files)

We can use the eslint-react-plugin. This issue will be mostly about which rules to use.
Assignee: nobody → lclark
Status: NEW → ASSIGNED
Attached patch Bug1249088.patch (obsolete) — Splinter Review
Attachment #8720415 - Flags: feedback?(jlong)
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
Attached patch Bug1249088.patch (obsolete) — Splinter Review
Patrick, would you mind giving this a quick review from an ESLint perspective?
Attachment #8720415 - Attachment is obsolete: true
Attachment #8720415 - Flags: feedback?(jlong)
Attachment #8723376 - Flags: review?(pbrosset)
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.
Keywords: checkin-needed
Blocks: 1251271
Attached patch Bug1249088.patchSplinter Review
Updated reviewer in commit message and created follow-up (Bug 1251271)
Attachment #8723376 - Attachment is obsolete: true
Attachment #8723613 - Flags: review+
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
Flags: needinfo?(lclark)
Keywords: checkin-needed
This is my fault, I just landed changes to .eslintrc in the meantime. I'll rebase and lad this one.
Flags: needinfo?(lclark)
https://hg.mozilla.org/mozilla-central/rev/9e162af63f4c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.