1249088 - Add eslint rules for React

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Lin Clark [:linclark]]

We can use the eslint-react-plugin. This issue will be mostly about which rules to use.

Comment 1

[Lin Clark [:linclark]]

Attachment: Bug1249088.patch

Comment 2

[Lin Clark [:linclark]]

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

Comment 3

[Lin Clark [:linclark]]

Attachment: Bug1249088.patch

Patrick, would you mind giving this a quick review from an ESLint perspective?

Comment 4

[Patrick Brosset <:pbro>]

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.

Comment 5

[Lin Clark [:linclark]]

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.

Comment 6

[Lin Clark [:linclark]]

Attachment: Bug1249088.patch

Updated reviewer in commit message and created follow-up (Bug 1251271)

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

[Patrick Brosset <:pbro>]

This is my fault, I just landed changes to .eslintrc in the meantime. I'll rebase and lad this one.

Comment 9

[Patrick Brosset <:pbro>]

https://hg.mozilla.org/integration/fx-team/rev/9e162af63f4c

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e162af63f4c