Closed
Bug 1263637
Opened 8 years ago
Closed 8 years ago
eslint 2 warnings for WebExtensions code
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox48 fixed)
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 | ||
Updated•8 years ago
|
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.3 - Apr 25
Assignee | ||
Comment 1•8 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•8 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
Comment 3•8 years ago
|
||
(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.)
Comment 5•8 years ago
|
||
(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•8 years ago
|
||
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•8 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 8•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f9bbe7db197
Assignee | ||
Comment 9•8 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/
Assignee | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b6a19e37c0e
Updated•8 years ago
|
Whiteboard: triaged
Assignee | ||
Updated•8 years ago
|
Attachment #8742345 -
Flags: review?(nalexander)
Comment 11•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8742345 -
Flags: review?(kmaglione+bmo) → review+
Comment 12•8 years ago
|
||
Comment on attachment 8742345 [details] MozReview Request: Bug 1263637 - eslint 2 warnings for WebExtensions code, r?kmag https://reviewboard.mozilla.org/r/47145/#review43975
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/72da827dde30
Keywords: checkin-needed
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/72da827dde30
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•