Closed Bug 1263637 Opened 8 years ago Closed 8 years ago

eslint 2 warnings for WebExtensions code

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.3 - Apr 25
Tracking Status
firefox48 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

(Whiteboard: triaged)

Attachments

(1 file)

After updating to eslint 2, I've noticed a number of warnings under browser [1] and toolkit [2].

I will investigate and fix these.

[1] https://gist.github.com/bobsilverberg/5063ce07bc51ae4e7d20392f8e08f9da
[2] https://gist.github.com/bobsilverberg/74b7e9edae46a09bfefe8e10b39fe24e
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.3 - Apr 25
Note that most of these warnings are because it is no longer legal to enclose a string in backticks if the only thing we need to escape is a double quote. So something like this:

throw new Error(`Internal error: "async" property must name the last parameter of the function.`);

is no longer legal (using the eslint `quotes` rule). It needs to be changed to use single quotes, like so:

throw new Error('Internal error: "async" property must name the last parameter of the function.');

There is an open issue for eslint [1] which is requesting a change that will re-allow the old format, and it looks like a PR may be submitted for that soon. I have asked a question in the issue to determine if a PR is forthcoming.

I think we should hold off on updating our code until this change lands, as I think we prefer to use backticks for escaping strings (as we often use them for template literals), rather than single quotes.

What do other people think?

[1] https://github.com/eslint/eslint/issues/5234#issuecomment-196530074
The PR that fixes the issue [1] was merged. It looks like they release every two weeks [2], and the last release (according to `npm view eslint time`) was April 4th, so maybe a new release will land with this fix next week.

I'll keep an eye on it and submit a patch to both the rules and the version of eslint when it is released.

I notice we have explicit rules for `quotes` in 4 locations:

* devtools/.eslintrc
* mobile/android/.eslintrc
* toolkit/components/extensions/.eslintrc
* toolkit/components/narrate/.eslintrc

I could just update the rules in `toolkit/components/extensions/.eslintrc` but considering that the old syntax is being deprecated it might be best to change all of them.

[1] https://github.com/eslint/eslint/commit/16787e1a7f2b1472458420d1ddb77f66a1bf7e00
[2] https://www.npmjs.com/package/eslint
See Also: → 1264971
(FWIW, it at least used to be the case that template strings were measurably slower than "normal" strings, and if you're not actually using any template string features, I don't really understand why you would use them over normal strings.)
(In reply to :Gijs Kruitbosch from comment #3)
> (FWIW, it at least used to be the case that template strings were measurably
> slower than "normal" strings, and if you're not actually using any template
> string features, I don't really understand why you would use them over
> normal strings.)

Template strings without any substitutions are converted to ordinary string
literals by the parser, so they won't be any slower.

There are a few places where we have template strings without any escapes or
newlines for consistency with the surrounding code, where the same types of
strings do. I think they can generally be changed to single or double quote
strings without issue, though.
I've submitted a patch that removes all the warnings by doing the following:

* update eslint to 2.8.0
* update the rules in `toolkit/components/extensions/.eslintrc` to allow backticks to be used to escape a string that contains quotes
* add comments to disable complexity warnings for two functions (and opened a bug to track refactoring those functions to remove that complexity)

As mentioned in my original comments, I can see why we would sometimes choose to use backticks over single quotes when needing to escape double quotes in strings, particularly when we are already using them for true template literals in the same file. If this is deemed unacceptable I can change this patch to replace the backticks with single quotes as well as back out the rule change which allows them to be used.
Comment on attachment 8742345 [details]
MozReview Request: Bug 1263637 - eslint 2 warnings for WebExtensions code, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47145/diff/1-2/
Whiteboard: triaged
Attachment #8742345 - Flags: review?(nalexander)
Comment on attachment 8742345 [details]
MozReview Request: Bug 1263637 - eslint 2 warnings for WebExtensions code, r?kmag

https://reviewboard.mozilla.org/r/47145/#review43923

Fine by me.
Attachment #8742345 - Flags: review?(nalexander) → review+
Attachment #8742345 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8742345 [details]
MozReview Request: Bug 1263637 - eslint 2 warnings for WebExtensions code, r?kmag

https://reviewboard.mozilla.org/r/47145/#review43975
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/72da827dde30
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.