1150273 - turn on React linting in eslint

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Dan Mosedale (:dmosedale, :dmose)]

Right now, the eslint configuration files for loop are only linting the generated .js files.  We need to stop linting those, and lint the JSX files instead.  

This might involve writing a Python or Node script to drive the linting, or it could conceivably involve moving our generated.js files to their own directory hierarchy.

To get benefit from this, we'll also need to select which rules we want to use, and turn them on:

https://github.com/yannickcr/eslint-plugin-react/blob/master/README.md#list-of-supported-rules

Comment 1

[Mark Banner (:standard8)]

Attachment: Use eslint for the react files for Loop.

I think rather than doing complex stuff for now, we should just turn on parsing of jsx and reap the benefits.

If we're going as far as pushing node into the build system (which I think we should aim for), then we should be able to get jsx there as well and hence we wouldn't need the .js files.

It might make turning on things like "no-trailing-spaces" more difficult for now, but I think having the extra jsx rules will help and it'll help with developing (e.g. running automatically with editors).

This patch turns on jsx parsing and adds it to the run script. I've enabled the rules where the code currently allows and where it makes sense. I've documented ones I don't think we'll ever want to set.

The other ones that are disabled, we'll need to do a little bit more work in the code to fix them if we want to.

Comment 2

[Mark Banner (:standard8)]

Attachment: Use eslint for the react files for Loop.

Ok, I just realised that the I'd turned on the react rules as warnings rather than errors, so the scripts wouldn't detect it (eslint doesn't fail on warning, and I meant for errors anyway).

Then I realised that run-all-loop-tests.sh doesn't exit if eslint fails, so I've included a fix for that as well.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Mark Banner (:standard8) from comment #1)
> Created attachment 8587245 [details] [diff] [review]
> Use eslint for the react files for Loop.
> 
> I think rather than doing complex stuff for now, we should just turn on
> parsing of jsx and reap the benefits. [...]

Sounds entirely reasonable; sold.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8587258 [details] [diff] [review]
Use eslint for the react files for Loop.

Review of attachment 8587258 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good; thanks so much for doing this.  I've noted a few small things in the comments; r=dmose.

::: browser/components/loop/.eslintrc
@@ +68,5 @@
>      "strict": 0,
> +    "yoda": 0,
> +    // eslint-plugin-react rules. These are documented at
> +    // <https://github.com/yannickcr/eslint-plugin-react#list-of-supported-rules>
> +    "react/jsx-quotes": [2, "double"],

I'd like to advocate for adding the third "avoid-escape" parameter here.  Having to use \ escapes in strings on the string delimiters is hard to read.

@@ +72,5 @@
> +    "react/jsx-quotes": [2, "double"],
> +    "react/jsx-no-undef": 2,
> +    "react/jsx-sort-props": 0,
> +    "react/jsx-uses-vars": 2,
> +    "react/no-did-mount-set-state": 0,

I assume we want this some day.  Add a comment explaining what needs to happen before we can turn it on?

@@ +74,5 @@
> +    "react/jsx-sort-props": 0,
> +    "react/jsx-uses-vars": 2,
> +    "react/no-did-mount-set-state": 0,
> +    "react/no-did-update-set-state": 2,
> +    "react/no-unknown-property": 2,

I'm a little surprised this works, since I'd sort of expect us to be using moz-specific DOM properties.  But I guess there's nothing wrong with using this until we hit an issue.

@@ +75,5 @@
> +    "react/jsx-uses-vars": 2,
> +    "react/no-did-mount-set-state": 0,
> +    "react/no-did-update-set-state": 2,
> +    "react/no-unknown-property": 2,
> +    "react/prop-types": 0,

I assume we want this some day.  Add a comment explaining what needs to happen before we can turn it on?

::: browser/components/loop/run-all-loop-tests.sh
@@ +15,5 @@
>  ESLINT=standalone/node_modules/.bin/eslint
>  if [ -x "${LOOPDIR}/${ESLINT}" ]; then
>    echo 'running eslint; see http://eslint.org/docs/rules/ for error info'
> +  (cd ${LOOPDIR} && ./${ESLINT} --ext .js --ext .jsx .)
> +  if [ $? == 1 ]; then

You might want !=0 here just to future-proof a bit.

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8587258 [details] [diff] [review]
Use eslint for the react files for Loop.

Review of attachment 8587258 [details] [diff] [review]:
-----------------------------------------------------------------

Nice changes, overall!

::: browser/components/loop/.eslintrc
@@ +70,5 @@
> +    // eslint-plugin-react rules. These are documented at
> +    // <https://github.com/yannickcr/eslint-plugin-react#list-of-supported-rules>
> +    "react/jsx-quotes": [2, "double"],
> +    "react/jsx-no-undef": 2,
> +    "react/jsx-sort-props": 0,

I think this means sorting prop defs in alphabetical order? I kinda like that practice!

::: browser/components/loop/content/js/roomViews.jsx
@@ +179,5 @@
> +        return (
> +          <DesktopRoomInvitationView
> +            roomStore={this.props.roomStore}
> +            dispatcher={this.props.dispatcher}
> +          />

while you're here, can you move the closing tag to hug the last property?

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> 
> ::: browser/components/loop/.eslintrc
> @@ +70,5 @@
> > +    // eslint-plugin-react rules. These are documented at
> > +    // <https://github.com/yannickcr/eslint-plugin-react#list-of-supported-rules>
> > +    "react/jsx-quotes": [2, "double"],
> > +    "react/jsx-no-undef": 2,
> > +    "react/jsx-sort-props": 0,
> 
> I think this means sorting prop defs in alphabetical order? I kinda like
> that practice!

FWIW, me too, as it makes it faster to visually scan longer lists of props and have confidence whether or not something is there.

Turning this on could happen in other bug, though.

Comment 7

[Mark Banner (:standard8)]

(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #4)
> > +    "react/no-did-mount-set-state": 0,
> 
> I assume we want this some day.  Add a comment explaining what needs to
> happen before we can turn it on?

I've only done generic "fix-it" comments for these, as that's all we need to do. It would have been too much to add all those in this bug.

Comment 8

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/872f80a098a4

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/872f80a098a4