Closed Bug 1454696 Opened Last year Closed Last year

Enforce using const over let on DevTools

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(firefox62 fixed)

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: yulia, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

We want to enable https://eslint.org/docs/rules/prefer-const for devtools. This issue was discussed here: https://github.com/devtools-html/rfcs/issues/44

this issue will required updating the eslint and replacing unnecessary instances of let in the codebase.
Severity: normal → enhancement
Blocks: dt-eslint-rules
No longer blocks: dt-polish-debt
Priority: -- → P3
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
The patches here will bitrot quickly :) There are about 150 violations that are not handled by `eslint --fix`, I moved them to a separate commit, might be worth quickly looking at it. 

For the new rule I settled on :

  "prefer-const": ["error", { "destructuring": "all" }],

destructuring: all is a bit more relaxed as it allows let {a, b, c} to be accepted even if one of the variables is never reassigned. 

We could remove it in a second step, that sums up to ~150 additional violations that can't be fixed by `eslint --fix`.
Comment on attachment 8982474 [details]
Bug 1454696 - Add eslint rule prefer-const to DevTools;

https://reviewboard.mozilla.org/r/248436/#review254686

Looks good!
Attachment #8982474 - Flags: review?(ystartsev) → review+
Comment on attachment 8982475 [details]
Bug 1454696 - Run eslint --fix for prefer-const;

https://reviewboard.mozilla.org/r/248438/#review254690

::: devtools/client/aboutdebugging/components/tabs/Target.js:40
(Diff revision 1)
>  
>    debug() {
> -    let { target, connect } = this.props;
> +    const { target, connect } = this.props;
>      let url = "about:devtools-toolbox?type=tab&id=" + target.outerWindowID;
>      if (connect.type == "REMOTE") {
> -      let {host, port} = connect.params;
> +      const {host, port} = connect.params;

hmm, its a little weird that this is done differently than others, in that it misses the space around the variable names. maybe something we can add the the eslint rules next time around?
Attachment #8982475 - Flags: review?(ystartsev) → review+
Comment on attachment 8982476 [details]
Bug 1454696 - Fix leftover issues for prefer-const;

https://reviewboard.mozilla.org/r/248442/#review254698

Nice work! \o/

::: devtools/client/jsonview/test/browser_jsonview_expand_collapse.js:28
(Diff revision 1)
>    await BrowserTestUtils.synthesizeMouseAtCenter(selector, {}, browser);
> -  countAfter = await getElementCount(".treeRow");
> +  let countAfter = await getElementCount(".treeRow");
>    ok(countAfter == 3, "There must be three rows");
>  
>    /* Test the "Expand All" button */
>    selector = ".jsonPanelBox .toolbar button.expand";

at some point it would be nice to avoid this reassignment in tests and use unique variable names / function scopes to do this
Attachment #8982476 - Flags: review?(ystartsev) → review+
Looks great so far! thanks for jumping on this!
(In reply to Yulia Startsev [:yulia] from comment #7)
> Comment on attachment 8982475 [details]
> Bug 1454696 - Run eslint --fix for prefer-const;
> 
> https://reviewboard.mozilla.org/r/248438/#review254690
> 
> ::: devtools/client/aboutdebugging/components/tabs/Target.js:40
> (Diff revision 1)
> >  
> >    debug() {
> > -    let { target, connect } = this.props;
> > +    const { target, connect } = this.props;
> >      let url = "about:devtools-toolbox?type=tab&id=" + target.outerWindowID;
> >      if (connect.type == "REMOTE") {
> > -      let {host, port} = connect.params;
> > +      const {host, port} = connect.params;
> 
> hmm, its a little weird that this is done differently than others, in that
> it misses the space around the variable names. maybe something we can add
> the the eslint rules next time around?

Do you mean enforcing { host, port } rather than {host, port} ? If so, yes I agree, having linting for this would be nice. (This is probably not the only spot where we have this in DevTools :) )

(In reply to Yulia Startsev [:yulia] from comment #9)
> Looks great so far! thanks for jumping on this!

Sure, thanks for reviewing :) A few failures on try, need to regenerate console stubs and to keep one more let in shared-head.js, but other than this this looks almost ready to land.
Comment on attachment 8982514 [details]
Bug 1454696 - Update console stubs to use const rather than let;

https://reviewboard.mozilla.org/r/248480/#review254744

Looks good to me, thanks Julian
Attachment #8982514 - Flags: review?(nchevobbe) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 446d0861840a478ec1ed3bb5ecaf86add056c2f9 -d 0e59c9a0cd3a: rebasing 466451:446d0861840a "Bug 1454696 - Add eslint rule prefer-const to DevTools;r=yulia"
rebasing 466452:9c051fdea802 "Bug 1454696 - Run eslint --fix for prefer-const;r=yulia"
merging devtools/client/framework/components/ToolboxTabs.js
merging devtools/client/framework/components/ToolboxToolbar.js
merging devtools/client/performance/modules/categories.js
merging devtools/client/performance/modules/logic/frame-utils.js
merging devtools/client/performance/test/browser_perf-tree-view-08.js
merging devtools/client/performance/test/helpers/synth-utils.js
merging devtools/client/performance/test/unit/test_profiler-categories.js
merging devtools/client/performance/test/unit/test_tree-model-07.js
merging devtools/client/performance/test/unit/test_tree-model-08.js
merging devtools/client/performance/test/unit/test_tree-model-09.js
merging devtools/client/shared/widgets/FlameGraph.js
merging devtools/client/webconsole/components/JSTerm.js
merging devtools/client/webconsole/local-dev/index.js
merging devtools/client/webconsole/test/mochitest/browser_jsterm_autocomplete_return_key.js
warning: conflicts while merging devtools/client/framework/components/ToolboxTabs.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/client/webconsole/components/JSTerm.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/client/webconsole/test/mochitest/browser_jsterm_autocomplete_return_key.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9dc44047042b
Add eslint rule prefer-const to DevTools;r=yulia
https://hg.mozilla.org/integration/autoland/rev/e8599930659e
Run eslint --fix for prefer-const;r=yulia
https://hg.mozilla.org/integration/autoland/rev/0add1bf599e5
Fix leftover issues for prefer-const;r=yulia
https://hg.mozilla.org/integration/autoland/rev/4a8ac9618893
Update console stubs to use const rather than let;r=nchevobbe
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.