eslint 2 warnings for WebExtensions code

RESOLVED FIXED in Firefox 48

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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)

Updated

2 years ago
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.3 - Apr 25
(Assignee)

Comment 1

2 years ago
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
(Assignee)

Comment 2

2 years ago
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
(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.
(Assignee)

Comment 6

2 years ago
Created attachment 8742345 [details]
MozReview Request: Bug 1263637 - eslint 2 warnings for WebExtensions code, r?kmag

Review commit: https://reviewboard.mozilla.org/r/47145/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47145/
Attachment #8742345 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 7

2 years ago
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.
(Assignee)

Comment 9

2 years ago
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/

Updated

2 years ago
Whiteboard: triaged
(Assignee)

Updated

2 years ago
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+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/72da827dde30
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.