Add no-implied-eval rule to eslint config

RESOLVED FIXED in Firefox 55

Status

Firefox Build System
Lint and Formatting
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: freddyb, Assigned: freddyb)

Tracking

Version 3
mozilla55

Firefox Tracking Flags

(firefox55 fixed)

Details

(URL)

MozReview Requests

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

Attachments

(1 attachment)

(Assignee)

Description

a year ago
We discourage use of eval (and string arguments to setInterval/setTimeout).
I suggest we set this in stone by adding this to our eslint config.
(Assignee)

Updated

a year ago
Assignee: nobody → fbraun
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
Looks good on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b2bf36674c7
(though MozReview thinks otherwise *shrugs*)
(Assignee)

Comment 4

a year ago
Tom: This eslint-rule is currently force-disabled in devtools/, would you be OK with enabling it as part of this bug?
FWIW, enabling the rule locally causes no errors or warnings.
Flags: needinfo?(ttromey)

Comment 5

a year ago
(In reply to Frederik Braun [:freddyb] from comment #4)
> Tom: This eslint-rule is currently force-disabled in devtools/, would you be
> OK with enabling it as part of this bug?
> FWIW, enabling the rule locally causes no errors or warnings.

Yes, I think it's a good idea.
From the comment in devtools/.eslintrc.js, I'd imagine that this rule was disabled by accident,
especially considering that "eval" itself is an error.
Flags: needinfo?(ttromey)

Comment 6

a year ago
mozreview-review
Comment on attachment 8859923 [details]
Bug 1358050: Add no-implied-eval rule to eslint config

https://reviewboard.mozilla.org/r/131980/#review134934

Thanks for the patch. This looks good to me overall.

nit: Unless there's a specific coding style for files in the toolkit/components/places/tests/browser folder, I would use 2 space indent, but I don't think this really matters for these test files.

Please enable the rule for devtools, and I would suggest requesting a final review from :Standard8.

::: browser/components/preferences/languages.js:22
(Diff revision 2)
>  
>    // Ugly hack used to trigger extra reflow in order to work around XUL bug 1194844;
>    // see bug 1194346.
>    forceReflow() {
>      this._activeLanguages.style.fontKerning = "none";
> -    setTimeout("gLanguagesDialog._activeLanguages.style.removeProperty('font-kerning')", 0);
> +    setTimeout(_ => {

nit: when there's no parameter we usually do '() =>' rather than defining a dummy parameter.
Attachment #8859923 - Flags: review?(florian)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8859923 - Flags: review?(florian)

Comment 8

a year ago
mozreview-review
Comment on attachment 8859923 [details]
Bug 1358050: Add no-implied-eval rule to eslint config

https://reviewboard.mozilla.org/r/131980/#review135214

Looks good, thanks for doing this. r=Standard8
Attachment #8859923 - Flags: review?(standard8) → review+
(Assignee)

Comment 9

a year ago
Still good on try. Please check in.
Keywords: checkin-needed

Comment 10

a year ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/402503d06db8
Add no-implied-eval rule to eslint config r=standard8
Keywords: checkin-needed

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/402503d06db8
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

a year ago
Blocks: 1359350

Updated

2 months ago
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.