Closed Bug 1159667 Opened 9 years ago Closed 9 years ago

Use eslint where jshint falls short

Categories

(Firefox OS Graveyard :: Gaia, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-master fixed)

RESOLVED FIXED
2.2 S14 (12june)
Tracking Status
b2g-master --- fixed

People

(Reporter: stas, Assigned: freddy)

References

Details

Attachments

(2 files)

eslint supports plugins so it might be a good solution for adding specialized checks to our pre-commit hook, like I did in bug 1159190.
Alternatively we should see if eslint can cover everything that jshint does today. If it can - migrating to it might be a good idea.
It seems possible and some very cursory research found multiple projects who did this. Yet, I think we should do this step by step.
Bringing in eslint should be the first, moving the jshint checks into eslint the second.
I've seen jshint developers to be very reactive to issues. We should look how eslint devs work :)

Also, we need to assess whether eslint works fine with our ES6 codebase -- I know they're making lots of progress towards this goal.
Attached file Github Pull Request
I got this working on taskcluster. ESLint already tests and finds things.
Next up, I want to provide a patch that whitelists all existing ESLint errors.
Assignee: nobody → fbraun
Status: NEW → ASSIGNED
Attachment #8605228 - Flags: review?(felash)
Comment on attachment 8605228 [details] [review]
Github Pull Request

Kevin, you've touched most of the files here and I could not find a clear module peer. Can you review this?
Attachment #8605228 - Flags: review?(kgrandon)
Comment on attachment 8605228 [details] [review]
Github Pull Request

Code looks ok to me, but I suppose a build peer should review this.

Also I noticed that the ESL tasks were failing. Should we land initially without the innerHTML validations?
Attachment #8605228 - Flags: review?(kgrandon) → review?(ricky060709)
Comment on attachment 8605228 [details] [review]
Github Pull Request

r+ for build part.
Attachment #8605228 - Flags: review?(ricky060709) → review+
Hey, just a heads up that I'll review today.
(In reply to Kevin Grandon :kgrandon from comment #6)
> Also I noticed that the ESL tasks were failing. Should we land initially
> without the innerHTML validations?

I was initially thinking of landing this after the failures are resolved, but you're right. Let's get this into tree and add the innerHTML validations later.
Comment on attachment 8605228 [details] [review]
Github Pull Request

I left comments on github.

My main concern is that we won't fix all existing uses of innerHTML, so I'd like to know the plan about this.

Also maybe we could enable right now some of the tests that pass (because we use jshint), but that could be in a separate bug too.
Attachment #8605228 - Flags: review?(felash) → review-
(In reply to Julien Wajsberg [:julienw] (PTO May 8th -> May 17th) from comment #10)
> Comment on attachment 8605228 [details] [review]
> Github Pull Request
> 
> I left comments on github.
> 
Noted, thanks!

> My main concern is that we won't fix all existing uses of innerHTML, so I'd
> like to know the plan about this.
> 

I'll whitelist existing instances, this will happen in another bug.
I intend to either whitelist by file or append a comment like "//TODO use new templating bug …". I'd prefer the comment approach because a whitelist on a per-file basis could allow additional violations to creep up. I'm also skeptical whether developers improving individual files would actually try to reduce the whitelist. I do not whish to keep a whitelist that has to be maintained along the way.

There will be a process to opt out of this check completely for corner cases. (See https://developer.mozilla.org/en-US/Firefox_OS/Security/Security_Automation)


> Also maybe we could enable right now some of the tests that pass (because we
> use jshint), but that could be in a separate bug too.

Yeah, I'd prefer that to happen in a different bug as well.
Comment on attachment 8605228 [details] [review]
Github Pull Request

Can you take another look, Julien?
Attachment #8605228 - Flags: review- → review?(felash)
Comment on attachment 8605228 [details] [review]
Github Pull Request

I think there are some little more work to do, especially about what we display to the user.
Attachment #8605228 - Flags: review?(felash) → review-
Comment on attachment 8605228 [details] [review]
Github Pull Request

I added an initial README.md in build/eslint, that I hope will suffice for now.

You didn't specifically say what you want in that file, so please let me know if there's more.
Attachment #8605228 - Flags: review- → review?(felash)
Attachment #8611181 - Flags: review?(eperelman)
The code looks good, but I couldn't try the hook yet (my tree is in the middle of a rebase with conflicts that I can't finish now because I'm at a conference). Is it ok if I finish the review monday ?
Comment on attachment 8611181 [details] [review]
[gaia-node-modules] mozfreddyb:bug-1159667-add-node-modules > mozilla-b2g:master

Redirecting to Kevin, as he has traditionally reviewed my changes to gaia-node-modules.
Attachment #8611181 - Flags: review?(eperelman) → review?(kgrandon)
Comment on attachment 8611181 [details] [review]
[gaia-node-modules] mozfreddyb:bug-1159667-add-node-modules > mozilla-b2g:master

I still think that the unsafe innerhtml plugin needs to be thought out more before landing, but I'm fine placing a review stamp on this.
Attachment #8611181 - Flags: review?(kgrandon) → review+
(In reply to Kevin Grandon :kgrandon from comment #18)
> Comment on attachment 8611181 [details] [review]
> [gaia-node-modules] mozfreddyb:bug-1159667-add-node-modules >
> mozilla-b2g:master
> 
> I still think that the unsafe innerhtml plugin needs to be thought out more
> before landing, but I'm fine placing a review stamp on this.

It's disabled in this PR anyway :)
This PR is about setting up the eslint checks and hook.
Comment on attachment 8605228 [details] [review]
Github Pull Request

I'm glad I tried it, because it didn't work ;)

r=me with the 3 comments fixed

thanks !
Attachment #8605228 - Flags: review?(felash) → review+
The gaia-node-modules patch needs manual check-in so I can update the gaia-patch with the correct revision id.

(I should have done this in two separate bugs…)
Comment on attachment 8605228 [details] [review]
Github Pull Request

I decided added the suggest xfail behavior after all.
Can you please review again, Julien? :)
Attachment #8605228 - Flags: review+ → review?(felash)
We need the xfail when running eslint in the Makefile too.

I'd be happy enough if you merge the xfail list and the jshint ignore list together. I don't know if you can specify --ignore-path twice...

The other option is (I think) to write a formatter to specify using -f.
Just an fyi in case it influences your landing decision - the jshint ignore list should be mostly gone in the next week or so, just waiting on alive to land his bootstrap patch, then another shared/ patch on top of that.
(In reply to Julien Wajsberg [:julienw] from comment #23)
> We need the xfail when running eslint in the Makefile too.

Good catch, I took this in the most recent commit.

(In reply to Kevin Grandon (PTO) :kgrandon from comment #24)
> Just an fyi in case it influences your landing decision - the jshint ignore
> list should be mostly gone in the next week or so, just waiting on alive to
> land his bootstrap patch, then another shared/ patch on top of that.

Thanks for the pointer! Turns out with the current setting and all rules disabled, eslint doesn't really need the jshintignore. ;)
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/bf10af35cab3e7b1ccdfefb3cc467f049d2e507c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S14 (12june)
So I'm surprised you asked for an additional review and still committed ?

Noone's harmed here since eslint is not checking anything yet but I'm surprised :)
Attachment #8605228 - Flags: review?(felash)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: