Closed Bug 1425521 Opened 2 years ago Closed 2 years ago

Put the JSTerm into the Console React app

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

At the moment, the jsterm (console input), lives as a separate vanilla JS app, which makes it disconnected from the console. 
Since we are planning to work on having the jsterm rewritten to react, we should go through a b

The first step of the rewrite of the jsterm to React/Redux is to integrate the current jsterm code into the console React app (i.e. in https://searchfox.org/mozilla-central/rev/110706c3c09d457dc70293b213d7bccb4f6f5643/devtools/client/webconsole/new-console-output/new-console-output-wrapper.js#197-206).

To do that, we can take some inspiration from https://github.com/ReactTraining/react-workshop/blob/master/subjects/ImperativeToDeclarative/solution.js to wrap the current code in a React element, so we don't have to work behind a pref to do all the jsterm work.
Blocks: 1425519
Blocks: 1425536
Depends on: 1425552
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: -- → P1
Blocks: 1136299
Here's an happy TRY push https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ce33352c016b6009378f0904326ca6597998e29.

I already removed some of the unnecessary function and properties in the new jsterm (everything related to the old sidebar/variableview).
Also, the fact that we return early for the browser console when `devtools.chrome.enabled` is false had some impact on some methods where we assumed that `this.inputNode` was always available. 
I think the patch makes sense and isn't too huge, but if you want to, I may put back the methods and properties like before and do some of those cleanups in follow ups.

The next move after this patch will be to put the execute function on the hud and pass everything that is needed by the jsterm as props, which should make the component easier to understand and refactor.
Can you do a talos comparison with/without the patch?
Comment on attachment 8964962 [details]
Bug 1425521 - Wrap JsTerm in a React component in new frontend; .

https://reviewboard.mozilla.org/r/233580/#review241634

This looks like a nice start! If you have a green try, then let's get it landed and iterate on it in follow ups

::: devtools/client/webconsole/webconsole.html
(Diff revision 3)
>              src="resource://devtools/client/webconsole/main.js"></script>
>    </head>
>    <body class="theme-sidebar" role="application">
>      <div id="app-wrapper" class="theme-body">
>        <div id="output-container" role="document" aria-live="polite"></div>
> -      <div id="jsterm-wrapper">

This node doesn't get added with the new component, but there's one reference to it in HTML docs in CSS:

- https://searchfox.org/mozilla-central/source/devtools/client/themes/webconsole.css#356

Unless if I'm missing something, that rule should be removed.

::: devtools/client/webconsole/webconsole.html
(Diff revision 3)
>      <div id="app-wrapper" class="theme-body">
>        <div id="output-container" role="document" aria-live="polite"></div>
> -      <div id="jsterm-wrapper">
> -        <div id="webconsole-notificationbox"></div>
> -        <div class="jsterm-input-container" style="direction:ltr">
> -          <div class="jsterm-stack-node">

This node doesn't get added with the new component, but there are two references to it in HTML docs in CSS:

- https://searchfox.org/mozilla-central/source/devtools/client/themes/webconsole.css#357
- https://searchfox.org/mozilla-central/source/devtools/client/themes/webconsole.css#394

Unless if I'm missing something, those should be removed.
Attachment #8964962 - Flags: review?(bgrinstead) → review+
Comment on attachment 8966474 [details]
Bug 1425521 - Move old jsterm file into webconsole/old; .

https://reviewboard.mozilla.org/r/235160/#review241638
Attachment #8966474 - Flags: review?(bgrinstead) → review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/18c6e8809c2b
Wrap JsTerm in a React component in new frontend; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/06f67baf2aed
Move old jsterm file into webconsole/old; r=bgrins.
https://hg.mozilla.org/mozilla-central/rev/18c6e8809c2b
https://hg.mozilla.org/mozilla-central/rev/06f67baf2aed
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Blocks: 1455513
No longer blocks: 1455513
Depends on: 1455513
Depends on: 1462055
Duplicate of this bug: 1425545
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.