Upgrade eslint to version 0.21.0 and enable no-undef rule

RESOLVED FIXED in Firefox 41

Status

defect
P3
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
mozilla41
Points:
2
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [tech-debt])

Attachments

(1 attachment, 1 obsolete attachment)

In looking at enabling the no-undef rule, I quickly realised that we wanted the new 0.21.0 version of eslint as that supports sharable configs (plus some other things we'll want later) - so that we can share the gecko rules more easily.
Mentor: standard8
Although this touches a lot of files, most of it is removing the old global declarations, and tidying up various things. Here's a few notes:

- Upgraded to 0.21.0 of eslint, and 0.2.3 of the react plugin. The plugin wasn't fully necessary but doesn't seem to hurt.
- New .eslintrc-gecko file included in the modules/, test/mochitest and test/xpcshell files.
- I'm avoiding /* global ... */ statements where possible. I think these mess up the readability quite a bit, and tend to get quite long - especially in gecko land.
- More globals defined in the .eslintrc-* files. I've tried to limit them to the scope they need to be where possible. We can probably improve, but take this as a first stab, see how it goes kinda thing.
- The xpcshell and mochitest directories define a bunch more globals - some for the functions relevant to those test types, the others are functions and variables defined in the head.js files. Where possible, I set things to read-only, but generally because of the head files some things need to be read-write.
- In the content unit tests, I've moved most of the "global" declarations of expect and other things into the describe bodies. eslint was picking up various cases where items hadn't been declared in files but the tests were passing because of "borrowing" globals from elsewhere. It seemed to make sense to move to a more specific definition.
- Enabling no-undef caught a couple of bugs in tests (yay!)
- I've also enabled linebreak-style (unix) and no-unneeded-ternary which are both new rules added with eslint 0.21 and pass without additional changes.

Something that crossed my mind, is that we might want to think about making eslint recognise "XPCOMUtils.define*Getter(this, "foo", ...)" as a global for "foo", this would simplify the gecko files. However, if its possible, that seems more like a long-term project, maybe something to keep in mind.
Attachment #8606238 - Flags: review?(dmose)
(In reply to Mark Banner (:standard8) from comment #1)
> Created attachment 8606238 [details] [diff] [review]
> Upgrade eslint to version 0.21.0 and enable no-undef rule.

I'll try to take a look at this on Monday.
(In reply to Mark Banner (:standard8) from comment #1)
> Created attachment 8606238 [details] [diff] [review]
> Upgrade eslint to version 0.21.0 and enable no-undef rule.
> 
> Although this touches a lot of files, most of it is removing the old global
> declarations, and tidying up various things. Here's a few notes:
> 
> - Upgraded to 0.21.0 of eslint, and 0.2.3 of the react plugin. The plugin
> wasn't fully necessary but doesn't seem to hurt.

Sounds good.  Excitingly, eslint 0.21.1 was released on Friday.  Feel free to land that as part of this, or not, as you wish.

> - New .eslintrc-gecko file included in the modules/, test/mochitest and
> test/xpcshell files.

Cool; nice code re-use here!

> - In the content unit tests, I've moved most of the "global" declarations of
> expect and other things into the describe bodies. eslint was picking up
> various cases where items hadn't been declared in files

Do you mean not declared in the test files themselves?   Or the code files under test?

I don't understand what the former would mean.

> but the tests were
> passing because of "borrowing" globals from elsewhere. It seemed to make
> sense to move to a more specific definition.

Assuming the answer to my above question is "the latter", it's not obvious to me what the win here is.  If eslint is already catching the undefined variables in the code files, then that's working as designed.  And if the variables are truly attached to the Window global that the test file is running in and are declared as such, then it's not clear to me that anything is going wrong.

> - I'm avoiding /* global ... */ statements where possible. I think these
> mess up the readability quite a bit, and tend to get quite long - especially
> in gecko land.

I could totally see how that would be true if one pulls that stuff down into the individual blocks.  OTOH, if we don't need to do that, it seems like having a long line at the top of the file doesn't particularly hurt readability.

> - More globals defined in the .eslintrc-* files. I've tried to limit them to
> the scope they need to be where possible. We can probably improve, but take
> this as a first stab, see how it goes kinda thing.

Sounds reasonable.

> - The xpcshell and mochitest directories define a bunch more globals - some
> for the functions relevant to those test types, the others are functions and
> variables defined in the head.js files. Where possible, I set things to
> read-only, but generally because of the head files some things need to be
> read-write.

Great.

> - Enabling no-undef caught a couple of bugs in tests (yay!)

W00t!

> - I've also enabled linebreak-style (unix) and no-unneeded-ternary which are
> both new rules added with eslint 0.21 and pass without additional changes.

Cool.

> Something that crossed my mind, is that we might want to think about making
> eslint recognise "XPCOMUtils.define*Getter(this, "foo", ...)" as a global
> for "foo", this would simplify the gecko files. However, if its possible,
> that seems more like a long-term project, maybe something to keep in mind.

I'd buy that.
(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #3)
> > - In the content unit tests, I've moved most of the "global" declarations of
> > expect and other things into the describe bodies. eslint was picking up
> > various cases where items hadn't been declared in files
> 
> Do you mean not declared in the test files themselves?   Or the code files
> under test?

The test files. I don't think we have any instances of the code files. Example: see feedbackViews_test.js in the code.

> > but the tests were
> > passing because of "borrowing" globals from elsewhere. It seemed to make
> > sense to move to a more specific definition.
> 
> Assuming the answer to my above question is "the latter", it's not obvious
> to me what the win here is.  If eslint is already catching the undefined
> variables in the code files, then that's working as designed.  And if the
> variables are truly attached to the Window global that the test file is
> running in and are declared as such, then it's not clear to me that anything
> is going wrong.

For us, it means we can't currently take some test files out and just run them in isolation. I know we don't normally do that though. I think its more the principle - we're relying on things we haven't explicitly declared in the test files, but we generally try to keep the individual tests as isolated from each other as possible.

Its also confusing, when eslint says "x" isn't defined (for which in most of the tests is generally defined as a non-global variable), but the test passes anyway.

> > - I'm avoiding /* global ... */ statements where possible. I think these
> > mess up the readability quite a bit, and tend to get quite long - especially
> > in gecko land.
> 
> I could totally see how that would be true if one pulls that stuff down into
> the individual blocks.  OTOH, if we don't need to do that, it seems like
> having a long line at the top of the file doesn't particularly hurt
> readability.

Except for some files, I think this could be 5-10 lines long easily. Maybe doesn't hurt readability in itself, but does give a lot more to keep up to date.
Updated patch for today's landings, as it turns out, this would have caught some unnecessary code before landing ;-)
Attachment #8606238 - Attachment is obsolete: true
Attachment #8606238 - Flags: review?(dmose)
Attachment #8608229 - Flags: review?(dmose)
(In reply to Mark Banner (:standard8) from comment #4
> 
> > > but the tests were
> > > passing because of "borrowing" globals from elsewhere. It seemed to make
> > > sense to move to a more specific definition.
> > 
> > Assuming the answer to my above question is "the latter", it's not obvious
> > to me what the win here is.  If eslint is already catching the undefined
> > variables in the code files, then that's working as designed.  And if the
> > variables are truly attached to the Window global that the test file is
> > running in and are declared as such, then it's not clear to me that anything
> > is going wrong.
> 
> For us, it means we can't currently take some test files out and just run
> them in isolation. I know we don't normally do that though. I think its more
> the principle - we're relying on things we haven't explicitly declared in
> the test files, but we generally try to keep the individual tests as
> isolated from each other as possible.
> 
> Its also confusing, when eslint says "x" isn't defined (for which in most of
> the tests is generally defined as a non-global variable), but the test
> passes anyway.

Oh, I see!  Another possibility might be a directly executed function closure around the entirety of each file.  But your proposal works too.

> > I could totally see how that would be true if one pulls that stuff down into
> > the individual blocks.  OTOH, if we don't need to do that, it seems like
> > having a long line at the top of the file doesn't particularly hurt
> > readability.
> 
> Except for some files, I think this could be 5-10 lines long easily. Maybe
> doesn't hurt readability in itself, but does give a lot more to keep up to
> date.

Good point.
Comment on attachment 8608229 [details] [diff] [review]
Upgrade eslint to version 0.21.0 and enable no-undef rule.

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

Looks good; r=dmose.  And disregard that last comment about the function closure; I wasn't looking in the right place.  :-)
Attachment #8608229 - Flags: review?(dmose) → review+
https://hg.mozilla.org/mozilla-central/rev/4975d5fbad0b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.