1454696 - Enforce using const over let on DevTools

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Yulia Startsev [:yulia] OOO until July 2026]

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.

Comment 1

[Julian Descottes [:jdescottes]]

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb26ff3e2ba848804dfc1ec884f9e087103b8a72

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1454696 - Add eslint rule prefer-const to DevTools;

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

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1454696 - Run eslint --fix for prefer-const;

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

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1454696 - Fix leftover issues for prefer-const;

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

Comment 5

[Julian Descottes [:jdescottes]]

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 6

[Yulia Startsev [:yulia] OOO until July 2026]

Comment on attachment 8982474 [details]
Bug 1454696 - Add eslint rule prefer-const to DevTools;

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

Looks good!

Comment 7

[Yulia Startsev [:yulia] OOO until July 2026]

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?

Comment 8

[Yulia Startsev [:yulia] OOO until July 2026]

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

Comment 9

[Yulia Startsev [:yulia] OOO until July 2026]

Looks great so far! thanks for jumping on this!

Comment 10

[Julian Descottes [:jdescottes]]

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

[Julian Descottes [:jdescottes]]

Attachment: Bug 1454696 - Update console stubs to use const rather than let;

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

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8982476 [details]
Bug 1454696 - Fix leftover issues for prefer-const;

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

Comment 13

[Julian Descottes [:jdescottes]]

New try with a fix for console stubs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d10a43bd1c2a1a2670becedd6a2b7e5154b88bfb

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 15

[Mozilla Autoland]

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)

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8982474 [details]
Bug 1454696 - Add eslint rule prefer-const to DevTools;

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

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8982475 [details]
Bug 1454696 - Run eslint --fix for prefer-const;

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

Comment 18

[Julian Descottes [:jdescottes]]

Comment on attachment 8982476 [details]
Bug 1454696 - Fix leftover issues for prefer-const;

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

Comment 19

[Julian Descottes [:jdescottes]]

Comment on attachment 8982514 [details]
Bug 1454696 - Update console stubs to use const rather than let;

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9dc44047042b
https://hg.mozilla.org/mozilla-central/rev/e8599930659e
https://hg.mozilla.org/mozilla-central/rev/0add1bf599e5
https://hg.mozilla.org/mozilla-central/rev/4a8ac9618893