Closed Bug 1358050 Opened 3 years ago Closed 3 years ago

Add no-implied-eval rule to eslint config

Categories

(Firefox Build System :: Lint and Formatting, enhancement)

3 Branch
enhancement
Not set

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: freddyb, Assigned: freddyb)

References

()

Details

Attachments

(1 file)

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: nobody → fbraun
Status: NEW → ASSIGNED
Looks good on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b2bf36674c7
(though MozReview thinks otherwise *shrugs*)
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)
(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 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)
Attachment #8859923 - Flags: review?(florian)
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+
Still good on try. Please check in.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/402503d06db8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1359350
Product: Testing → Firefox Build System
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.