Open Bug 1517907 Opened 6 years ago Updated 2 years ago

console helpers should not override same-name top-level let/const variables

Categories

(DevTools :: Console, defect, P2)

Product:
DevTools
Component:
Console
Type:
defect

Tracking

(Not tracked)

Status:
ASSIGNED

People

(Reporter: ntim, Assigned: nchevobbe)

Details

Attachments

(1 file, 2 obsolete files)

Less confusing example
263.06 KB, image/png
Details
Attached image Screen Shot 2019-01-05 at 03.07.05.png (obsolete) —
Left is Firefox. Chrome has the expected behaviour here by overriding $ and throwing the error.
Attached image Less confusing example —
Nicolas, Is there something that could be done here ?
Flags: needinfo?(nchevobbe)
Attachment #9034526 - Attachment is obsolete: true
The issue isn't really that we can't override `$`: if you use `var $ = 1` (or declare a global), your example will work. It's related to how we deal with `let` assignment.
Flags: needinfo?(nchevobbe)
Summary: Console doesn't let me override $ shortcut → Console doesn't let me override $ shortcut with `let`
Priority: -- → P2

What if the let assignment internally got changed to var assignment, somehow? Is that possible?

Note that this is what Chrome does, obviously.

Simple test:

  1. Load data:text/html,<script>setInterval(() => console.log($('body')), 1000)</script>
    => Errors will be logged that $ is undefined.
  2. In the console execute let $ = x => document.querySelector(x);
    => The function is defined and the <body> element will be logged.
  3. In the console execute let $ = 1;
    => Errors will be logged that $ is undefined. There is no error logged that $ cannot be redeclared!

Furthermore, this also applies to any other global variables defined via let in the console.

Sebastian

Flags: needinfo?(nchevobbe)

The best place to do that would be in the getEvalInput() function, I think.

And the simplest solution would be to simply run a string.replace(/^let\s+/, "var "); before returning. A more complex solution would parse the input and replace all global let assignments by var assignments.

Sebastian

(In reply to Sebastian Zartner [:sebo] from comment #4)

What if the let assignment internally got changed to var assignment, somehow? Is that possible?
Furthermore, this also applies to any other global variables defined via let in the console.

We have Bug 1580891 about this. My concern was that it's not a simple things to do. let and var are different things in regards to their scope:

var myVar = 1;
let myLet = 2;
console.log(globalThis.myVar, globalThis.myLet) // -> 1 undefined

Hence the discussion we had around for UI needs: https://github.com/firefox-devtools/ux/issues/30

Note that this is what Chrome does, obviously.

They are doing some things to be able to re-declare let and const variables, but it's not a simple let -> var replacement

let a = 1
let a = 2

is still throwing in Chrome console (which is what we want), and variables assigned with let are not put on globalThis.

For this specific bug, we might want to adjust what we do in bindCommands (devtools/server/actors/webconsole/eval-with-debugger.js#584-619), or/and directly in executeInGlobalWithBindings

Flags: needinfo?(nchevobbe)

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #6)

(In reply to Sebastian Zartner [:sebo] from comment #4)

Note that this is what Chrome does, obviously.

They are doing some things to be able to re-declare let and const variables, but it's not a simple let -> var replacement

I already assumed that, otherwise you'd already have implemented that. :-)

let a = 1
let a = 2

is still throwing in Chrome console (which is what we want), and variables assigned with let are not put on globalThis.

If executed in one command, it throws, in two it doesn't.
Also, this throws:

var a = 1
let a = 2

I think, the related Chromium issue is https://crbug.com/542594. Anyway, you obviously already put some more thought into this than I did. Thank you for adding the reference links! I just wanted to state the supposedly simple solution for this.

Sebastian

Severity: normal → S3
Summary: Console doesn't let me override $ shortcut with `let` → console helpers should not override same-name top-level let/const variables
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3c8034b8e9a3 [devtools] Don't add webconsole helper bindings if variable exists. r=devtools-reviewers,ochameau.

I'm looking into this
Here's a push with a potential fix: https://treeherder.mozilla.org/jobs?repo=try&revision=ca7c8139b13a06c1e5b7205136a6f5eea86f65a1

Flags: needinfo?(nchevobbe)

Comment on attachment 9342335 [details]
Bug 1517907 - [devtools] Don't add webconsole helper bindings if variable exists. r=#devtools-reviewers.

Revision D182717 was moved to bug 1385198. Setting attachment 9342335 [details] to obsolete.

Attachment #9342335 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: