Add ESLint to the build & test process for Loop

RESOLVED FIXED in Firefox 40

Status

Hello (Loop)
Client
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: pdehaan, Assigned: dmose)

Tracking

unspecified
mozilla40
Points:
2

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [tech-debt])

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (obsolete)
Thanks for the report. I don't see anything major here, so I think we can leave these tidy ups until after we land something in nightly, when we'll also have a bit more context and time to decide what level of static checking (and which tool(s)) that we want going forward.
Severity: normal → trivial
OS: Mac OS X → All
Priority: -- → P5
Hardware: x86 → All
Whiteboard: [tech-debt]

Updated

3 years ago
backlog: --- → Fx36+

Updated

3 years ago
backlog: Fx36+ → Fx37+
Its a great idea, which we want to take further and get ESLint in the tree for at least Loop. We may start with jsxhint as an intermediary given our current use of react.

Therefore morphing the bug, and we'll fix the warnings/errors when we get it included.
Severity: trivial → normal
backlog: Fx37+ → tech-debt
Priority: P5 → P2
Summary: A few errors/warnings reported via ESLint against loop-client → Add ESLint to the build & test process for Loop
I've tagged comment 0 as obsolete as so many things have changed since that run.
(Assignee)

Updated

2 years ago
Assignee: nobody → dmose
(Assignee)

Comment 4

2 years ago
Created attachment 8586447 [details] [diff] [review]
bootstrap eslint for loop content JS, fix a found bug
Comment on attachment 8586447 [details] [diff] [review]
bootstrap eslint for loop content JS, fix a found bug

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

rs=me Note that I don't know much about eslint but it seems like this is mostly getting the initial setup ready so we can start enforcing it more later.
Attachment #8586447 - Flags: review+
(Assignee)

Comment 6

2 years ago
Created attachment 8586466 [details] [diff] [review]
bootstrap eslint for loop content JS, fix a found bug, rs=MattN
Attachment #8586447 - Attachment is obsolete: true
Attachment #8586466 - Flags: review+
(Assignee)

Comment 7

2 years ago
Indeed, this is simply bootstrapping the various files to ignore relevant things, and turning off most of the rules, so that we don't need to do a giant code-whack all-at-once to get things started.

That said, even these meager settings found one bug, and I've included a fix for that here.

To start taking advantage of this, all you have to do (once) is go into browser/components/loop/standalone and do "make install", which will npm install the eslint bits you need.

I've got some commented out bits in run-all-loop-browser-tests.sh which will start running these whenever someone executes that script and happens to have eslint installed locally, from the above step, which I'll get review on shortly.
(Assignee)

Comment 8

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/91b6a396bf7c
(Assignee)

Comment 9

2 years ago
Note also that your favorite editor/IDE may now, possibly with a bit of configuration, be able to give you instantaneous feedback based on the configuration files now in the tree. http://eslint.org/docs/user-guide/integrations has the details.
(Assignee)

Comment 10

2 years ago
I've set up a cron job on the dev server which runs eslint on an updated copy of fx-team every hour, and sends me any error output.  If anyone else would like on the list, please let me know. :-)

Obviously, this is just for the bootstrapping phase; we'll soon want to find a faster and more robust feedback mechanism (eg travis-ci, treeherder, ...).  Spinoff bugs to come.  If you're interested in getting email for this when it fails, feel free to let me know.  :-)
(Assignee)

Comment 11

2 years ago
Wrote a brief blog post about this: https://dmose2.wordpress.com/2015/04/01/bootstrapping-eslint-for-hello/
(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #7)
> I've got some commented out bits in run-all-loop-browser-tests.sh which will
> start running these whenever someone executes that script and happens to
> have eslint installed locally, from the above step, which I'll get review on
> shortly.

Adding leave-open as it seems like this review is still pending, and I'm assuming that's going ot happen in this bug.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/91b6a396bf7c
(Assignee)

Comment 14

2 years ago
Created attachment 8586990 [details] [diff] [review]
add ESLint to run-all-loop-tests; update docs, rs=Standard, DONTBUILD
Attachment #8586990 - Flags: review+
(Assignee)

Comment 15

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/c6b002187f58
(Assignee)

Updated

2 years ago
Iteration: --- → 40.1 - 13 Apr
Points: --- → 2
Keywords: leave-open
Target Milestone: --- → mozilla40
https://hg.mozilla.org/mozilla-central/rev/c6b002187f58
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
People interested in this ticket may be interested in Bug 1177933.
You need to log in before you can comment on or make changes to this bug.