[WIP] Use devtools-reps-0.19.0 by nchevobbe · Pull Request #4915 · firefox-devtools/debugger

nchevobbe · 2017-12-13T23:16:24Z

Updating to the latest devtools-reps means that we need to change how we instantiate ObjectInspector.

This is a wip because :

~~devtools-reps-0.17.0 is not published yet~~
~~I didn't checked the tests~~
~~The Popup does not seem to work.~~

Yet, watch expressions and scopes are fine.

jasonLaster · 2017-12-14T21:02:28Z

This is looking good @nchevobbe. Nice work w/ createObjectClient

nchevobbe · 2017-12-15T19:04:22Z

So the only thing with this patch is that the preview show you the root (which is then autoexpanded)

before:

after:

nchevobbe · 2017-12-15T19:04:47Z

Also: photon colors !

nchevobbe · 2017-12-17T12:42:15Z

Here's the "flashing" issue I talked you about @jasonLaster :

Basically, it's the time we have to wait for properties to be loaded. This is a big case here since we target document, which has a lot of things.
We could prevent to render until we have everything loaded to make it better, but the latency to actually show something in the preview might be an issue ? (probably not more than what we were doing previously).

nchevobbe · 2017-12-17T12:43:01Z

Okay so yeah, just tested the same case in Nightly, and we do have this latency on document, the only thing being we don't show anything until we have all properties loaded.

jasonLaster · 2017-12-18T14:41:47Z

@nchevobbe i don't mind delaying showing the preview. It should be snappy on most objects.

I suppose we could also try some animations too. Perhaps there's something fun we could do with animating the yellow highlight? dunno

nchevobbe · 2017-12-18T17:06:30Z

okay, nice
So do we have clear next steps to make this PR landed ?

jasonLaster · 2017-12-18T17:08:06Z

Lets treat the preview loading behavior as a next step. And just focus on getting the tests passing now

nchevobbe · 2017-12-18T17:55:53Z

Sure, CI is failing because of Flow. Let me fix this now

nchevobbe · 2017-12-18T19:06:36Z

Erf, at this point, we would break preview for React and Immutable https://github.com/devtools-html/debugger.html/pull/4915/files#diff-2e80e25aeaf08281d8427771196d61e1R101 . 😕

@MikeRatcliffe do you think you could have a look and see how we could do thing in the ObjectInspector to autoLoad properties and get them back in some way, so we don't have to change much code for the Preview here ?

jasonLaster · 2017-12-18T19:07:33Z

@nchevobbe happy to look in a bit

jasonLaster · 2017-12-19T14:23:57Z

Did a quick look

Immutable did not seem to regress
the preview font is bigger
the source tree seems clipped
previewing a react component breaks STR: go to this site and add a breakpoint in App.js

MikeRatcliffe · 2017-12-19T21:10:37Z

When we are trying to preview this inside a React component the following error is thrown:

Warning: setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op. Please check the code for the ObjectInspector component. 13 warning.js:33
printWarning warning.js:33
warning warning.js:57
getInternalInstanceReadyForUpdate ReactUpdateQueue.js:46
enqueueSetState ReactUpdateQueue.js:207
ReactComponent.prototype.setState ReactBaseClasses.js:62
setExpanded index.js:310:8

Not sure why it is unmounted at this point.

nchevobbe · 2017-12-19T21:25:42Z

Okay, I guess we need some ObjectInspector changes here

nchevobbe · 2017-12-24T15:04:31Z

source tree is now fixed.
We use devtools-reps-0.17.0

I think the error about setState is when we hover quickly a variable and then mouseleave.
This causes the ObjectInspector to be mounted, rendered, making him fetching the first level properties, and thus trying to do a setState. But at this point, the OI is unmounted because we not longer hover the variable in the debugger.
I am not sure how this can be fixed, but I think I want to keep the same behavior as before , which means we need to modify reps.

nchevobbe · 2018-01-09T16:49:18Z

@jasonLaster The popup is working just like before now, but i did not tested the React and Immutable cases.
I didn't fixed the mochitests and did not refactor the loadObjectProperties action much because I wanted to check if you are okay with this approach.

This is based on firefox-devtools/devtools-core#884, which will probably be in a 0.19.0 reps package.

There is also a minor styling issue (see firefox-devtools/devtools-core#886)

codehag

looks quite good! a couple of comments regarding type annotation

codehag · 2018-01-10T14:41:31Z


+let DebuggerClient;
+
+function createObjectClient(grip: Object) {


could we use the Grip type here from flow-typed/debugger-html? https://github.com/devtools-html/debugger.html/blob/master/flow-typed/debugger-html.js#L235

codehag · 2018-01-10T14:44:10Z

    } = this.props;
+    const root = createNode(null, expression, expression, { value });
+
+    if (!nodeIsPrimitive(root) && value.actor && !loadedObjects[value.actor]) {


before, the value was potentially null -- is that no longer the case? maybe we can type this?

codehag · 2018-01-10T14:44:19Z

+      expression,
+      loadObjectProperties,
+      loadedObjects
    } = this.props;


The function has changed to componentWillMount, so this.props will be representing the previous props. do we want to use this.props or nextProps?

codehag · 2018-01-10T14:46:47Z


-  getChildren(root: Object, getObjectProperties: Function) {
-    const actors = {};
+  getChildren(root: Object) {


is root of type Node? https://github.com/devtools-html/debugger.html/blob/master/src/utils/sources-tree/types.js#L14

it is, thanks for pointing to the type

codehag · 2018-01-10T14:48:02Z

@@ -103,16 +123,18 @@ export class Popup extends Component<Props> {
  }

  renderObjectPreview(expression: string, root: Object, extra: Object) {


also maybe here we can use the node type for root: https://github.com/devtools-html/debugger.html/blob/master/src/utils/sources-tree/types.js#L14

it will need to be moved to a more general type

codehag · 2018-01-10T14:48:59Z

-        // TODO: See https://github.com/devtools-html/debugger.html/issues/3555.
-        getObjectEntries={actor => {}}
-        loadObjectEntries={grip => {}}
+        createObjectClient={grip => createObjectClient(grip)}


codehag · 2018-01-10T14:50:03Z

-            // TODO: See https://github.com/devtools-html/debugger.html/issues/3555.
-            getObjectEntries={actor => {}}
-            loadObjectEntries={grip => {}}
+            createObjectClient={grip => createObjectClient(grip)}


codehag · 2018-01-10T14:50:28Z

    >
      <ObjectInspector
        autoExpandDepth={0}
+        createObjectClient={[Function]}


its just a code deletion party! hurray!

nchevobbe · 2018-01-11T09:22:24Z

I think everything is working fine now, except:

it requires devtools-reps-0.19.0 (on its way)
it certainly breaks ChromeScope
and mochitest who rely on LOAD_OBJECT_PROPERTIES (the action was changed to only set popup object properties)

jasonLaster · 2018-01-11T16:04:28Z

i can git rm ChromeScope it was an experiment for CDP but hasnt been touched in awhile

nchevobbe · 2018-01-16T16:45:44Z

Okay, so everything is working now, devtools-reps-0.19.0 has been published, we're in a good place.
Except for those tests.
A bunch of them are using something like await waitForDispatch(dbg, "LOAD_OBJECT_PROPERTIES"); which awaits forever since we don't have this action anymore (or even something that could replace it).

I'm not sure how to tackle this, should I edit all the tests so they don't rely on this action, but more on the UI ? Something like "wait until this .tree element has more than 1 node" ?

That's how it's done in the console, but here it will differ from what the test are doing 🤔

@jasonLaster do you have an idea how I can tackle this without rewriting all the tests ?

jasonLaster · 2018-01-16T17:00:30Z

yep - lets wait for the dom to have rendered loaded scope variables...

nchevobbe · 2018-01-18T13:24:05Z

okay, i'm down to 3 mochitests failing.
At least 2 of them are failing because the PR breaks the Scopes "remapping", I need to check what's going on

nchevobbe · 2018-01-18T13:54:34Z

okay I get it.
The ObjectInspector keeps a Map of cached nodes, which is never cleaned up.
When remapping the scope, we use the same OI instance, and so auto-expanded node, which were cached, are not updated.

This means one more change in Reps to fix this :/

nchevobbe · 2018-01-18T18:17:31Z

@jasonLaster everything is working now (with firefox-devtools/devtools-core#905), so this PR is good for review.
It seems that there are conflicts I'll resolve tomorrow

Since we don't have the LOAD_OBJECT_PROPERTIES anymore, we need to find other ways to wait for the properties to be loaded in tests.

nchevobbe · 2018-01-19T07:45:30Z

not sure what's going in with retext-spell, but aside from that everything is good (with the pending reps PR).
@jasonLaster this is ready for review

wldcordeiro · 2018-01-19T08:02:58Z

@nchevobbe you need to add setPopupObjectProperties to /assets/dictionary.txt I believe.

wldcordeiro

This is packed with so much good stuff! I'm excited to see this land.

Also, sort the dictionary.

wldcordeiro · 2018-01-19T08:12:48Z

+let DebuggerClient;
+
+function createObjectClient(grip: Grip) {
+  return DebuggerClient.createObjectClient(grip);


Mochitest is failing here because the assignment in the onConnect below hadn't happened yet. Should you put a guard in to check if the client isn't undefined?

I think it was failing because of https://bugzilla.mozilla.org/show_bug.cgi?id=1430799, which now landed in mozilla-central.
Locally, all the mochitest pass with --verify :)

Ah good to know. I saw the failure complaining about undefined and figured it was some async funkiness. Guess we just need to update the CI image?

i'm not familiar with this, but if this mean the CI image needs to be updated to pick up mozilla-central changes, then yes :)

@nchevobbe you can update the .travis.yml w/ the new mc sha

nchevobbe · 2018-01-19T08:15:02Z

@nchevobbe you need to add setPopupObjectProperties to /assets/dictionary.txt I believe.

Yes ! Thanks, I fixed it in the PR (and took this opportunity to sort the dictionary as well).

jasonLaster · 2018-01-19T15:35:33Z

+  node.click();
+  return waitUntil(
+    () => objectInspector.querySelectorAll(".node").length !== properties
+  );


shouldn't we wait till it is the same?

can we move this to a helper?

nope, since we expand a node, the result should be that the objectInspector has a different number of nodes

jasonLaster · 2018-01-19T15:35:33Z

-  return new Promise(r => setTimeout(r, 3000000));
-}
-
 function getScopeNodeLabel(dbg, index) {


jasonLaster · 2018-01-19T15:35:34Z

-  );
+async function waitForLoadedScopes(dbg) {
+  await waitUntil(() => findElement(dbg, "scopes"));
+  const scopes = findElement(dbg, "scopes");


this can be const scopes = await waitUntil(() => findElement(dbg, "scopes"));

mh, I'll try, but I looked up the waitUntil function, and it was resolving with true, not the result of the condition.
Though, I did not try what you suggest here, so I will

this makes test fail because scopes.querySelector is not a function.

This is explained by https://searchfox.org/mozilla-central/rev/2031c0f517185b2bd0e8f6f92f9491b3410c1f7f/devtools/client/framework/test/shared-head.js#453-462 , where waitUntil resolves with true.

oh, i think we have waitForElement :)

jasonLaster · 2018-01-19T15:42:13Z

+  return waitUntil(
+    () => objectInspector.querySelectorAll(".node").length !== properties
+  );
 }


Perhaps we can have helpers like this in mochitest/head.js?

async function toggleExpression() { const objectInspector = findElement(dbg, "expressionNode", index).closest(".object-inspector"); await toggleObjectInspectorNode(dbg, objectInspector); } async function toggleObjectInspectorNode(dbg, objectInspector) { const getNumProperties = () => objectInspector.querySelectorAll(".node").length; const numProperties = getNumProperties(); node.click(); return waitUntil(() => numProperties !== getNumProperties()); }

yeah, sounds like a good idea

jasonLaster · 2018-01-19T15:42:14Z

+  node.click();
+  return waitUntil(
+    () => objectInspector.querySelectorAll(".node").length !== properties
+  );


function toggleNode(dbg, index) { const objectInspector = findElement(dbg, "scopeNode", index).closest(".object-inspector"); return toggleObjectInspectorNode(dbg, objectInspector); }

jasonLaster · 2018-01-19T15:47:17Z

+        const properties = await onLoadItemProperties;
+        setPopupObjectProperties(value, properties);
+      }
+    }


this looks like it could be moved to a util in the debugger or reps project.

I'm thinking this because it pulls in a bunch of dependencies and is pretty complicated...

could we move some of this logic to the actions/pause/setupPopup...

nchevobbe force-pushed the reps branch 2 times, most recently from 0ecce42 to 6cc0e63 Compare December 14, 2017 19:39

nchevobbe force-pushed the reps branch from 6cc0e63 to 06db7d3 Compare December 24, 2017 14:55

nchevobbe force-pushed the reps branch 2 times, most recently from 160e7ef to e8c4c79 Compare January 8, 2018 10:04

nchevobbe changed the title ~~[WIP] Use devtools-reps-0.16.0~~ [WIP] Use devtools-reps-0.18.0 Jan 9, 2018

nchevobbe force-pushed the reps branch 3 times, most recently from 48e7ee4 to 53d8d7a Compare January 9, 2018 16:46

codehag approved these changes Jan 10, 2018

View reviewed changes

nchevobbe force-pushed the reps branch from 53d8d7a to 0cf7985 Compare January 11, 2018 09:17

nchevobbe changed the title ~~[WIP] Use devtools-reps-0.18.0~~ [WIP] Use devtools-reps-0.19.0 Jan 16, 2018

nchevobbe force-pushed the reps branch from 0cf7985 to 0c1d469 Compare January 18, 2018 13:20

nchevobbe mentioned this pull request Jan 18, 2018

Flush nodes cache when roots prop is updated. firefox-devtools/devtools-core#905

Merged

nchevobbe force-pushed the reps branch from 0c1d469 to f3d5974 Compare January 18, 2018 18:14

Update devtools-reps to 0.19.0

e60f009

nchevobbe requested a review from jasonLaster January 18, 2018 18:17

nchevobbe force-pushed the reps branch from f3d5974 to 19ac29f Compare January 19, 2018 07:35

nchevobbe added 2 commits January 19, 2018 08:44

Adapt debugger to the new reps API design

a08f0b5

Adapt tests to new ObjectInspector.

cc7e209

Since we don't have the LOAD_OBJECT_PROPERTIES anymore, we need to find other ways to wait for the properties to be loaded in tests.

nchevobbe force-pushed the reps branch from 19ac29f to cc7e209 Compare January 19, 2018 07:44

wldcordeiro approved these changes Jan 19, 2018

View reviewed changes

Add setPopupObjectProperties to dictionary.

02d5dba

Also, sort the dictionary.

wldcordeiro reviewed Jan 19, 2018

View reviewed changes

Update CI image to latest mozilla-central

0c06551

jasonLaster reviewed Jan 19, 2018

View reviewed changes

nchevobbe and others added 2 commits January 19, 2018 19:33

Move toggle function to head.js

617e40b

tweaks

cfe7be7

jasonLaster merged commit 6d4220e into firefox-devtools:master Jan 19, 2018

jasonLaster pushed a commit that referenced this pull request Jan 24, 2018

Use devtools-reps-0.19.0 (#4915)

fdfcd10

Fischer-L mentioned this pull request Feb 1, 2018

5241 - [Preview] Show empty values #5279

Merged

nchevobbe deleted the reps branch October 25, 2018 15:20


		let DebuggerClient;

		function createObjectClient(grip: Object) {

		@@ -103,16 +123,18 @@ export class Popup extends Component<Props> {
		}

		renderObjectPreview(expression: string, root: Object, extra: Object) {

Conversation

nchevobbe commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasonLaster commented Dec 14, 2017

Uh oh!

nchevobbe commented Dec 15, 2017

Uh oh!

nchevobbe commented Dec 15, 2017

Uh oh!

nchevobbe commented Dec 17, 2017

Uh oh!

nchevobbe commented Dec 17, 2017

Uh oh!

jasonLaster commented Dec 18, 2017

Uh oh!

nchevobbe commented Dec 18, 2017

Uh oh!

jasonLaster commented Dec 18, 2017

Uh oh!

nchevobbe commented Dec 18, 2017

Uh oh!

nchevobbe commented Dec 18, 2017

Uh oh!

jasonLaster commented Dec 18, 2017

Uh oh!

jasonLaster commented Dec 19, 2017

Uh oh!

MikeRatcliffe commented Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nchevobbe commented Dec 19, 2017

Uh oh!

nchevobbe commented Dec 24, 2017

Uh oh!

nchevobbe commented Jan 9, 2018

Uh oh!

codehag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchevobbe commented Jan 11, 2018

Uh oh!

jasonLaster commented Jan 11, 2018

Uh oh!

nchevobbe commented Jan 16, 2018

Uh oh!

jasonLaster commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nchevobbe commented Jan 18, 2018

Uh oh!

nchevobbe commented Jan 18, 2018

Uh oh!

nchevobbe commented Jan 18, 2018

Uh oh!

nchevobbe commented Jan 19, 2018

Uh oh!

wldcordeiro commented Jan 19, 2018

nchevobbe commented Dec 13, 2017 •

edited

Loading

MikeRatcliffe commented Dec 19, 2017 •

edited

Loading

jasonLaster commented Jan 16, 2018 •

edited

Loading

jasonLaster Jan 19, 2018 •

edited

Loading