Closed Bug 1190293 Opened 4 years ago Closed 4 years ago

Update Loop's use of Eslint to v1.0.0 and eslint-plugin-react to 3.2.x

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
Points:
2

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Iteration:
42.3 - Aug 10
Tracking Status
firefox42 --- fixed

People

(Reporter: standard8, Assigned: standard8)

Details

Attachments

(1 file, 1 obsolete file)

These libraries have now been updated, and we should update to the latest - there's a few more rules, plus a few improvements.

Release notes:

http://eslint.org/blog/2015/07/eslint-1.0.0-released/
https://github.com/yannickcr/eslint-plugin-react/blob/master/History.md

From the eslint perspective, the main changes are that they no longer have default rules. There's a recommended set which now has to be included, but that's a reduced subset of the original defaults.

I've used the changes listed at https://github.com/eslint/eslint/commit/e3e9dbd9876daf4bdeb4e15f8a76a9d5e6e03e39#diff-b01a5cfd9361ca9280a460fd6bb8edbbR1 to rebuild our list. Although we have to list more, I think I like it as its more explicit.
See comment 0 for main backstory.

For the no-extend-native rule, I decided to special-case the one instance where we break it with an explanation as to why. Then if we break it elsewhere, we'll need to special case or not break it ;-)
Attachment #8642306 - Flags: review?(dmose)
Comment on attachment 8642306 [details] [diff] [review]
Upgrade Loop's use of eslint to 1.0.x and the eslint-plugin-react to 3.1.x

Andrei, if you could review this, that would be much appreciated!
Attachment #8642306 - Flags: review?(dmose) → review?(andrei.br92)
Comment on attachment 8642306 [details] [diff] [review]
Upgrade Loop's use of eslint to 1.0.x and the eslint-plugin-react to 3.1.x

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

Looking forward to having even more of these rules active.

::: browser/components/loop/.eslintrc
@@ +72,4 @@
>      "no-multi-spaces": 0,         // TBD.
> +    "no-multi-str": 2,
> +    "no-native-reassign": 2,
> +    "no-new": 0,                  // TODO: set to 2

Setting this does not trigger any new issues.

@@ +78,5 @@
> +    "no-new-wrappers": 2,
> +    "no-octal-escape": 2,
> +    "no-process-exit": 2,
> +    "no-proto": 2,
> +    "no-return-assign": 0,        // TODO: set to 2

This is a really quick fix. 

https://github.com/mozilla/gecko-dev/blob/56f294a5ecf9eb6b57aa6346f00b66fe2eb48318/browser/components/loop/test/desktop-local/roomStore_test.js#L155

No need to modify the accumulator just return the result.

@@ +105,3 @@
>      "spaced-comment": [2, "always"],
>      "strict": [2, "function"],
> +    "yoda": [2, "never"],

What's wrong with Yoda? Haha.

::: browser/components/loop/content/shared/js/utils.js
@@ +452,1 @@
>      Uint8Array.prototype.slice = Uint8Array.prototype.subarray;

But do we actually use this? I couldn't find it in the code.
Attachment #8642306 - Flags: review?(andrei.br92) → review-
(In reply to Andrei Oprea [:andreio] from comment #3)
> ::: browser/components/loop/.eslintrc
> @@ +72,4 @@
> > +    "no-new": 0,                  // TODO: set to 2
> 
> Setting this does not trigger any new issues.

Ah, it used to cause issues, but look like we added an exception to test/.eslintrc a while ago that fixed it.

> @@ +78,5 @@
> > +    "no-return-assign": 0,        // TODO: set to 2
> 
> This is a really quick fix. 

There used to be more of those as well ;-)

> @@ +105,3 @@
> >      "spaced-comment": [2, "always"],
> >      "strict": [2, "function"],
> > +    "yoda": [2, "never"],
> 
> What's wrong with Yoda? Haha.

Consistency ;-)

> ::: browser/components/loop/content/shared/js/utils.js
> @@ +452,1 @@
> >      Uint8Array.prototype.slice = Uint8Array.prototype.subarray;
> 
> But do we actually use this? I couldn't find it in the code.

Its used in cryto.js (_splitIVandCipherText).
Updated for comments.
Attachment #8642992 - Flags: review?(andrei.br92)
Comment on attachment 8642992 [details] [diff] [review]
Upgrade Loop's use of eslint to 1.0.x and the eslint-plugin-react to 3.1.x. v2

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

Awesome. Ship it!
Attachment #8642992 - Flags: review?(andrei.br92) → review+
Attachment #8642306 - Attachment is obsolete: true
3.2.x of eslint-plugin-react is now out, so we agreed over irc to do the bump.
Summary: Update Loop's use of Eslint to v1.0.0 and eslint-plugin-react to 3.1.x → Update Loop's use of Eslint to v1.0.0 and eslint-plugin-react to 3.2.x
https://hg.mozilla.org/mozilla-central/rev/fc1efb158f9b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.