turn on React linting in eslint

RESOLVED FIXED in Firefox 40

Status

Hello (Loop)
Client
P2
normal
Rank:
20
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dmose, Assigned: standard8)

Tracking

unspecified
mozilla40
Points:
2
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [tech-debt])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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
Flags: firefox-backlog+
(Assignee)

Comment 1

3 years ago
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.

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.
Attachment #8587245 - Flags: review?(dmose)
Attachment #8587245 - Flags: feedback?(mdeboer)
(Assignee)

Comment 2

3 years ago
Created attachment 8587258 [details] [diff] [review]
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.
Attachment #8587245 - Attachment is obsolete: true
Attachment #8587245 - Flags: review?(dmose)
Attachment #8587245 - Flags: feedback?(mdeboer)
Attachment #8587258 - Flags: review?(dmose)
Attachment #8587258 - Flags: feedback?(mdeboer)
(Assignee)

Updated

3 years ago
Assignee: nobody → standard8
Iteration: --- → 40.1 - 13 Apr
Points: --- → 2
Flags: qe-verify-
Whiteboard: [tech-debt]
(Assignee)

Updated

3 years ago
Blocks: 1150632
(Reporter)

Comment 3

3 years ago
(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.
(Reporter)

Comment 4

3 years ago
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.
Attachment #8587258 - Flags: review?(dmose) → review+
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?
Attachment #8587258 - Flags: feedback?(mdeboer) → feedback+
(Reporter)

Comment 6

3 years ago
(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.

Updated

3 years ago
Rank: 20
Priority: -- → P2
(Assignee)

Comment 7

3 years ago
(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.
(Assignee)

Comment 8

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/872f80a098a4
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla40
https://hg.mozilla.org/mozilla-central/rev/872f80a098a4
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.