[Scopes] expanded properties are not updated while stepping #2509

arthur801031 · 2017-03-31T06:25:14Z

Associated Issue: #2394

Summary of Changes

Assign new values to the cache actor if it goes stale

Test Plan

pause and expand an object
step
check if expanded object has updated

Screenshots/Videos (OPTIONAL)

arthur801031 · 2017-03-31T06:36:40Z

Hi @jasonLaster

The expanded properties would not be updated while stepping is because we're getting the properties from the cache actor instead of the current item's properties. The solution that I got fixes this issue but there is one drawback: we're not actually updating the cache actor; instead, we modify the cache actor locally and return this object to getChildren.

Please let me know if there is a better way to tackle this issue. 😄

bomsy · 2017-03-31T09:59:24Z

src/utils/object-inspector.js

+        }
+      }
+    }
+    return thisActor;


Looking good! If we move this change to the ObjectInspector.js here instead.
Then we can do this.props.setActors(actors); after, which will set the cached actor.

Great suggestion! I thought using this.props.setActors(actors); in render would cause an error, but I was wrong! Thank you! 😊

bomsy · 2017-03-31T10:00:17Z

Looking good!

codecov · 2017-03-31T13:05:12Z

Codecov Report

Merging #2509 into master will increase coverage by 1.12%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2509      +/-   ##
==========================================
+ Coverage   53.24%   54.36%   +1.12%     
==========================================
  Files          51       51              
  Lines        1959     1970      +11     
  Branches      393      396       +3     
==========================================
+ Hits         1043     1071      +28     
+ Misses        916      899      -17

Impacted Files	Coverage Δ
src/utils/object-inspector.js	`76.47% <100%> (+21.52%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da1b5cf...8557ef9. Read the comment docs.

bomsy · 2017-03-31T13:40:07Z

src/components/Editor/Preview.js

@@ -100,6 +100,7 @@ class Preview extends Component {
      onDoubleClick: () => {},
      loadObjectProperties,
      getActors: () => ({}),
+      setActors: () => {},


this will return undefined, should be setActors: () => ({}) instead

bomsy · 2017-03-31T13:40:26Z

src/components/SecondaryPanes/Expressions.js

@@ -162,6 +162,7 @@ class Expressions extends React.Component {
          this.editExpression(expression, options),
        loadObjectProperties,
        getActors: () => ({}),
+        setActors: () => {},


bomsy · 2017-03-31T13:50:36Z

src/components/shared/ObjectInspector.js

@@ -122,6 +122,26 @@ const ObjectInspector = React.createClass({
  getChildren(item: ObjectInspectorItem) {
    const { getObjectProperties } = this.props;
    const { actors } = this;
+    const key = item.path;
+
+    if (item.contents.value && item.contents.value.preview && actors[key]) {


tiny note, just for security ... actors && actors[key]

jasonLaster · 2017-03-31T14:40:33Z

src/components/shared/ObjectInspector.js

+        }
+      }
+      actors[key] = thisActor;
+      this.props.setActors(actors);


I'm wondering if we can simplify the cache significantly.

The way i see it working is that it would keep a key of IDs that are expanded and that's it. This way, values don't need to be synced...

bomsy · 2017-04-03T19:09:11Z

src/utils/object-inspector.js

+      }
+      index++;
+    });
+    return thisActor;


adding actors[key] = thisActor; above this line should update actorCache

Also , the actors[key] only goes stale across the ObjectInspector component loads (i.e during stepping), therefore across internal component renders we should still be fine doing return actors[key]if it exists, due to performance reasons.

arthur801031 · 2017-04-04T02:08:15Z

Hi @bomsy and @jasonLaster

I've reverted to previous implementation where the code logic for detecting/updating actors is in shared/ObjectInspector.js. This has 2 benefits:

I think this implementation is more reliable than the one that was in src/utils/object-inspector.js because I had to use multiple if conditions to avoid setting function objects.
We can update actors in shared/ObjectInspector.js, but we can't do that in src/utils/object-inspector.js.

To @bomsy:
I'm not sure if we can know when component loads (i.e during stepping) or internal components being render. What we have right now checks whether or not actors goes stale. If it does, we update actors.

bomsy · 2017-04-04T13:49:53Z

ok looking and testing ..

jasonLaster · 2017-04-04T14:56:18Z

src/components/shared/ObjectInspector.js

+        actors[key] = thisActor;
+        this.props.setActors(actors);
+      }
+    }


can this move to utils/object-inspector and get some unit tests?

jasonLaster

couple simplifying comments

jasonLaster · 2017-04-05T04:11:39Z

src/utils/tests/object-inspector.js

@@ -221,4 +222,63 @@ describe("promises", () => {
    const node = getPromiseProperties(promise);
    expect(node.contents.value.type).to.eql("3");
  });
+
+  it("update actors when necessary", () => {
+    const item = {


can we remove some of the data from this fixture so that it is much smaller. for instance, do we need the entire value? Can we remove preview's contents?

Yes, please check my new commit :)

jasonLaster · 2017-04-05T04:12:05Z

src/utils/object-inspector.js

@@ -181,6 +181,22 @@ function getChildren(
  // node would be a new instance every render.
  const key = item.path;
  if (actors && actors[key]) {
+    if (item.contents.value && item.contents.value.preview) {


can we move this code to an updateActor helper function?

No problem 🙂

bomsy · 2017-04-05T10:25:49Z

While testing this yesterday, noticed that if a property does not exist it does not get added.

I think this will require us going back to get the getProperties again, which invalidates the expanded cache,

I think we can land what we have atm, and improve on this.

…devtools#2509) * Fix expanded properties are not updated while stepping * Revert yarn.lock * Move code logic to ObjectInspector * Made a few corrections * Retrieve property values * Remove code from previous commit * Revert to previous implementation * Revert yarn.lock * Move code logic to object-inspector and add a unit test * Simplify code

arthur801031 added 2 commits March 31, 2017 13:53

Fix expanded properties are not updated while stepping

96fc2a6

Revert yarn.lock

47f48cd

bomsy reviewed Mar 31, 2017

View reviewed changes

Move code logic to ObjectInspector

1e61143

bomsy reviewed Mar 31, 2017

View reviewed changes

Made a few corrections

a6ef1b4

jasonLaster reviewed Mar 31, 2017

View reviewed changes

arthur801031 added 2 commits April 1, 2017 17:40

Retrieve property values

b1ed6d1

Remove code from previous commit

79bb50b

bomsy reviewed Apr 3, 2017

View reviewed changes

arthur801031 added 2 commits April 4, 2017 09:46

Revert to previous implementation

ac94d3a

Revert yarn.lock

1e41c11

jasonLaster reviewed Apr 4, 2017

View reviewed changes

jasonLaster and others added 2 commits April 4, 2017 13:40

Merge branch 'master' into properties

ed50775

Move code logic to object-inspector and add a unit test

7932d8f

jasonLaster reviewed Apr 5, 2017

View reviewed changes

Simplify code

8557ef9

arthur801031 changed the title ~~WIP - [Scopes] expanded properties are not updated while stepping~~ [Scopes] expanded properties are not updated while stepping Apr 5, 2017

jasonLaster approved these changes Apr 5, 2017

View reviewed changes

jasonLaster merged commit 1223468 into firefox-devtools:master Apr 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Scopes] expanded properties are not updated while stepping #2509

[Scopes] expanded properties are not updated while stepping #2509

arthur801031 commented Mar 31, 2017 •

edited

arthur801031 commented Mar 31, 2017

bomsy Mar 31, 2017 •

edited

arthur801031 Mar 31, 2017 •

edited

bomsy commented Mar 31, 2017

codecov bot commented Mar 31, 2017 •

edited

bomsy Mar 31, 2017

bomsy Mar 31, 2017

bomsy Mar 31, 2017

jasonLaster Mar 31, 2017

bomsy Apr 3, 2017

bomsy Apr 3, 2017 •

edited

arthur801031 commented Apr 4, 2017

bomsy commented Apr 4, 2017

jasonLaster Apr 4, 2017

jasonLaster left a comment

jasonLaster Apr 5, 2017

arthur801031 Apr 5, 2017

jasonLaster Apr 5, 2017

arthur801031 Apr 5, 2017

bomsy commented Apr 5, 2017

[Scopes] expanded properties are not updated while stepping #2509

[Scopes] expanded properties are not updated while stepping #2509

Conversation

arthur801031 commented Mar 31, 2017 • edited

Summary of Changes

Test Plan

Screenshots/Videos (OPTIONAL)

arthur801031 commented Mar 31, 2017

bomsy Mar 31, 2017 • edited

Choose a reason for hiding this comment

arthur801031 Mar 31, 2017 • edited

Choose a reason for hiding this comment

bomsy commented Mar 31, 2017

codecov bot commented Mar 31, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bomsy Apr 3, 2017 • edited

Choose a reason for hiding this comment

arthur801031 commented Apr 4, 2017

bomsy commented Apr 4, 2017

Choose a reason for hiding this comment

jasonLaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bomsy commented Apr 5, 2017

arthur801031 commented Mar 31, 2017 •

edited

bomsy Mar 31, 2017 •

edited

arthur801031 Mar 31, 2017 •

edited

codecov bot commented Mar 31, 2017 •

edited

bomsy Apr 3, 2017 •

edited