1358050 - Add no-implied-eval rule to eslint config

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Frederik Braun [:freddy]]

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.

Comment 1

[Frederik Braun [:freddy]]

Attachment: Bug 1358050: Add no-implied-eval rule to eslint config

Review commit: https://reviewboard.mozilla.org/r/131980/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131980/

Comment 2

[Frederik Braun [:freddy]]

Comment on attachment 8859923 [details]
Bug 1358050: Add no-implied-eval rule to eslint config

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131980/diff/1-2/

Comment 3

[Frederik Braun [:freddy]]

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

Comment 4

[Frederik Braun [:freddy]]

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.

Comment 5

[Tom Tromey :tromey]

(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.

Comment 6

[Florian Quèze [:florian]]

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.

Comment 7

[Frederik Braun [:freddy]]

Comment on attachment 8859923 [details]
Bug 1358050: Add no-implied-eval rule to eslint config

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131980/diff/2-3/

Comment 8

[Mark Banner (:standard8)]

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

Comment 9

[Frederik Braun [:freddy]]

Still good on try. Please check in.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/402503d06db8
Add no-implied-eval rule to eslint config r=standard8

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/402503d06db8