Closed Bug 1263637 Opened 4 years ago Closed 4 years ago
eslint 2 warnings for Web
58 bytes, text/x-review-board-request
After updating to eslint 2, I've noticed a number of warnings under browser  and toolkit . I will investigate and fix these.  https://gist.github.com/bobsilverberg/5063ce07bc51ae4e7d20392f8e08f9da  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  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?  https://github.com/eslint/eslint/issues/5234#issuecomment-196530074
The PR that fixes the issue  was merged. It looks like they release every two weeks , 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.  https://github.com/eslint/eslint/commit/16787e1a7f2b1472458420d1ddb77f66a1bf7e00  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.
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)
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/
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
You need to log in before you can comment on or make changes to this bug.