Deferred object getters evaluation #569

zinoviev · 2017-02-26T18:42:47Z

PR for #390

gaearon · 2017-03-02T00:48:28Z

Can I ask you to rebuild test/example/target.js so that it doesn't show up as a conflict?
Thanks!

zinoviev · 2017-03-02T14:47:24Z

gaearon · 2017-03-02T18:48:32Z

Great. I’m going to look at this and a few other PRs in a batch later next week.
Thank you so much for your work on this!

zinoviev · 2017-03-03T14:19:56Z

@gaearon
Sure. Where are couple of things you need to know

I don't implement handling getters in TreeView yet. If general approach on evaluating and passing value between backend and frontend is fine it can be added in no time
If you pass getter itself as a prop it won't be included in dehydrate check, so (...) rendering wont happens.

var obj = {
  name: 'Foo',
  get upper() {
   return this.name.toUpperCase();
 }
}

<Works prop={obj} />

<NotWorks prop={obj.upper} />

I believe this case can be handled too, it must be somewhere in agent/backend code, but I don't investigate this part yet.

On the other hand if user pass getter as prop do we really need to care about it's deferred eval, cause probably it's value will be used inside component logic immediately.

jaredly

looks great!

jaredly · 2017-03-08T22:56:12Z

frontend/DataView/Getter.js

+  }
+
+  render() {
+    return (<div style={style} onClick={this.handleClick.bind(this)}>(…)</div>);


nit: don't need the parens

jaredly · 2017-03-08T22:58:18Z

yeah I don't think we have to worry about the deferred eval of passing the getter in directly (and indeed I don't think we'd be able to)

gaearon · 2017-03-08T23:18:31Z

I don't implement handling getters in TreeView yet. If general approach on evaluating and passing value between backend and frontend is fine it can be added in no time

Let’s do this then? It would help me if you also added some screenshots so I knew better how to test this.

zinoviev · 2017-03-08T23:39:56Z

@gaearon k, Will do it later this week

zinoviev · 2017-03-15T22:57:33Z

@gaearon @jaredly Updated PR, was not so trivial as I expect. Spend some time dealing with updates logic on Node component and store decorator.

To test:

Pass object with getter as prop
On details panel and in tree you should see (...)
Click on (...)
Getter value should be instead of (...) in details and tree

zinoviev · 2017-03-15T23:00:56Z

frontend/Store.js

+    var last = data.path.length - 1;
+    var propObj = data.path.slice(0, last).reduce((obj_, attr) => obj_ ? obj_[attr] : null, props);
+    propObj[data.path[last]] = data.value;
+    var newProps = assign({}, props);


Props must be new object or bounded Node wont rerenders

Seems like tangential unrelated issue (worth raising). Why is this specific to getters (and not just any prop mutations)?

Reason why - you don't mutate original props on requesting getter value. If you edit some prop(for example text) in devtools, it forwards changes to original page component. Original component props mutates and new set of props goes back to devtools store, immutable collection recreates, UI rerenders etc

But on requesting getter value we pass just singular value and do not make any changes to original prop. So this part of code needed to make frontend store's data in sync and refresh UI nicely.

But why do we need this line:
var newProps = assign({}, props);

Data stored in immutable map on store._nodes But node props is not immutable map, it just a plain JS object. In Node.js (component node on left panel) where is SCU check

shouldComponentUpdate(nextProps: PropsType) { return nextProps !== this.props; }

So if we just swap value inside this object UI wont refreshes correclty

Thanks for explaining, this makes sense.

gaearon · 2017-03-27T13:25:17Z

Just a heads up: I'm in the middle of some other work but I'll get back to this as soon as I can. Likely within one or two weeks.

gaearon · 2017-04-20T13:52:20Z

How does this work with updates? Say I have a getter that is backed by an instance field. That field changes. Is there any way to re-evaluate the getter? Should there be?

gaearon · 2017-04-20T13:55:22Z

I think ideally there should be a way to re-evaluate the getter so that you don’t end up with stale values forever. What do you think?

gaearon · 2017-04-20T13:59:20Z

Let's take this example from #390 (comment):

var proto = {
  get upperName() { return this.name.toUpperCase()}
};

var instance = Object.create(proto);
instance.name = 'Foo';

When I am editing it, here is what I see:

Why does the getter disappear?

gaearon

Let’s figure out a strategy for re-evaluating a getter, and also fix the issue I described in gif.

gaearon · 2017-04-20T14:01:38Z

(Thanks for your work on this. I’m sorry I haven’t been very responsive lately. I think this is a good start but let’s ensure the feature works well in most cases so that people learn to trust it. Otherwise, even if we fix it in the future, people will avoid it because it did’t match their expectations before.)

Note I’ve pushed to your branch so you shouldn’t worry about merging with master.

zinoviev · 2017-04-20T14:17:41Z

@gaearon I will check about hiding, must some caveat with object nesting

About re-evaluating: don't think we need worry at all due to unidirectional data flow nature and immutable state. If user change something what means we get new set of props and new eval buttons (...) Where is no way to for getter value to be staled - it's not evaluated yet or it's correct and actual. I will check it next as I back to code

gaearon · 2017-04-20T14:26:02Z

Interesting. I tried a simple example where getter and setter write to a backing property, like

let obj = {
  get lol() {
    return this._lol;
  }
  set lol(val) {
    this._lol = val;
  }
}

and I don't think editing worked for me.

zinoviev · 2017-04-20T18:13:25Z

@gaearon I take a look, seems updates on backend side affects getters not the way I expect. Will look further on this issues

…tters # Conflicts: # frontend/PropVal.js

zinoviev · 2017-05-04T16:10:40Z

@gaearon ping
Now editing props reset getters, so it can't be stalled.

Also your example with update object with getter defined on prototype should be fine

gaearon · 2017-05-04T16:26:34Z

Cool! I’ll review when I get the time.

…tters # Conflicts: # agent/Agent.js # frontend/Store.js

zinoviev · 2017-05-31T11:35:00Z

@gaearon Merge master and resolve conflict, also I've added samples to prop tester mini-app

gaearon · 2017-06-02T15:00:15Z

agent/dehydrate.js

@@ -165,4 +173,11 @@ function dehydrate(data: Object, cleaned: Array<Array<string>>, path?: Array<str
  }
 }

+function isPropertyGetter(obj, prop) {
+  /* eslint-disable no-proto */
+  const descriptor = Object.getOwnPropertyDescriptor(obj.hasOwnProperty(prop) ? obj : obj.__proto__, prop);


Why is this check one level deep? Could it be on __proto__.__proto__, __proto__.__proto__.__proto__, etc? Do we need a loop here?

My fail, will fix asap!

gaearon · 2017-06-02T15:06:01Z

When withSetter._value is edited, withGetter.upperName collapses even if it was previously shown. This is good because it could've been invalidated, but can be annoying if you're inside some deep object which you know doesn't change, and it collapses on every single edit to something else.

I wonder if maybe it's better to keep it around, but show a (...) after the value to trigger another evaluation? What do you think?

gaearon · 2017-06-06T15:25:32Z

@zinoviev What are your thoughts on this? I wonder if we can look at how this compares to how Chrome displays getters.

zinoviev · 2017-06-06T16:11:33Z

@gaearon
The same way as we do actually - reset all getters on any edit, no extra buttons in UI. The only difference - they reset state only for the object you make changes, others in inspector does not updates.

I'm not sure it's actually intended, perhaps just the way devtools works.

Getter doesn't have to be pure functions:

var foo = {
  get wow() {
    return this._val.toUpperCase();
  },
  _val: 'fofofo',
};

var bar = {
  get bla() {
    return foo._val;
  }
}

So it's easy to make the outdated value in this case and if you want real value you must print object on console or add new watch.

As far I remember, in react devtools on any edit we get a new set of props every time user edit anything - immutable state and unidirectional flow for the rescue. So I think react devtools have a more reliable implementation for now.

I don't like refresh buttons, imo explicit is better than implicit and neither chrome has ones. We can try to implement chrome like behavior for single object updates, but thinks it's out of scope for this PR as I don't actually touch updates passing mechanism and it can be major changes, not sure if it worths.

gaearon · 2017-06-06T16:14:13Z

I'm mostly concerned about this making edits inside large trees unusable if any parent is a getter. I understand that getter could mutate other objects but limiting this to the same object would make sure the feature doesn't break existing workflows for people. It's a compromise.

zinoviev · 2017-06-08T09:06:33Z

@gaearon Like this?

gaearon · 2017-06-29T18:42:22Z

This looks like it! Apologies, I don't have the time to review right now but we'll try to get back to it. There's just too many things going on at the same time.

zinoviev · 2017-07-09T09:37:16Z

@gaearon ping, I've make requested changes and some more additions

gaearon · 2017-07-09T18:47:51Z

I'm sorry I haven't looked yet. We got a lot of changes through but this one is complex enough that it keeps slipping through the cracks. I'm adding it to my todos next week, and will try to review. Thank you again for working on this.

zinoviev · 2017-09-26T21:35:04Z

@gaearon ping, any chances for review?

dbenson24 · 2018-05-24T11:51:51Z

@gaearon any chance this can get reviewed sometime?
#390 and #893

inoyakaigor · 2018-06-28T16:02:50Z

So what's with the PR? Problem still actual.
@gaearon ping

Vannevelj · 2019-01-16T11:00:57Z

Can this get reviewed? It's breaking on an important part of my code that I'd really like to inspect.

bvaughn · 2019-08-20T16:35:52Z

React DevTools has been rewritten and recently launched a new version 4 UI 🎉 The source code for this rewrite was done in a separate repository and is being merged into the main React repo.

Thank you for taking the time to contribute to this extension 🙇‍♂️

Because this PR is for the version code base, I am closing it. If you'd like to contribute to the new extension, please visit the new repository.

Deferred object getters evaluation

a2877ae

facebook-github-bot added GH Review: review-needed CLA Signed labels Feb 26, 2017

zinoviev mentioned this pull request Feb 26, 2017

Display "..." on getters to defer their evaluation #390

Closed

Igor Zinoviev and others added 2 commits March 2, 2017 15:19

Fix merge conflict

3db945c

Fix flow errors

e9f2c02

jaredly approved these changes Mar 8, 2017

View reviewed changes

zinoviev added 2 commits March 13, 2017 23:18

Merge remote-tracking branch 'github/master' into defer-getters

9158344

show evaluated getter in TreeView

8fffed5

zinoviev commented Mar 15, 2017

View reviewed changes

gaearon mentioned this pull request Apr 10, 2017

Bugfix: Correctly calculate Overlay position when inspecting a node within iframe(s) #552

Merged

gaearon mentioned this pull request Apr 20, 2017

Vigorous perusal of large Redux stores leads to non-responsiveness and eventually OOM #599

Closed

Merge master

6275882

gaearon suggested changes Apr 20, 2017

View reviewed changes

Merge commit '31a7a87b288fed3a5a625db1de5a9fdf84eafa0a' into defer-ge…

c3da494

…tters # Conflicts: # frontend/PropVal.js

zinoviev added 2 commits May 31, 2017 13:06

Merge commit '8bac134feaa470b29af309dc019d7640909dedbd' into defer-ge…

e7d6e0f

…tters # Conflicts: # agent/Agent.js # frontend/Store.js

Fix merge errors, add test samples

ce60b1c

gaearon mentioned this pull request Jun 2, 2017

Do Not Invoke Getters Implicitly - Hide it behind explicit UI #775

Closed

gaearon reviewed Jun 2, 2017

View reviewed changes

Naive implementation

b4cd08f

zinoviev added 3 commits June 16, 2017 17:26

Don’t collapse getters outside edit path

b59ebf5

Find getter inside prototype chain

a64d1c9

Merge branch 'master' into defer-getters

c5d6eb2

zinoviev mentioned this pull request Jun 17, 2017

Flow errors with npm #817

Closed

Make getters work for context and state

0d2565a

klode mentioned this pull request Mar 9, 2018

Inspector fails to display objects that use getters #893

Closed

johnrichter mentioned this pull request Jun 27, 2018

Computed properties evaluated before property assignment mobxjs/mobx#1608

Closed

bvaughn closed this Aug 20, 2019

Deferred object getters evaluation #569

Deferred object getters evaluation #569

Conversation

zinoviev commented Feb 26, 2017

gaearon commented Mar 2, 2017

zinoviev commented Mar 2, 2017

gaearon commented Mar 2, 2017

zinoviev commented Mar 3, 2017 • edited Loading

jaredly left a comment

Choose a reason for hiding this comment

jaredly Mar 8, 2017

Choose a reason for hiding this comment

jaredly commented Mar 8, 2017

gaearon commented Mar 8, 2017

zinoviev commented Mar 8, 2017

zinoviev commented Mar 15, 2017 • edited Loading

zinoviev Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

gaearon Apr 20, 2017

Choose a reason for hiding this comment

zinoviev Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

gaearon Apr 20, 2017

Choose a reason for hiding this comment

gaearon commented Mar 27, 2017

gaearon commented Apr 20, 2017

gaearon commented Apr 20, 2017

gaearon commented Apr 20, 2017

gaearon left a comment

Choose a reason for hiding this comment

gaearon commented Apr 20, 2017

zinoviev commented Apr 20, 2017

gaearon commented Apr 20, 2017

zinoviev commented Apr 20, 2017

zinoviev commented May 4, 2017

gaearon commented May 4, 2017

zinoviev commented May 31, 2017

gaearon Jun 2, 2017

Choose a reason for hiding this comment

zinoviev Jun 6, 2017

Choose a reason for hiding this comment

gaearon commented Jun 2, 2017

gaearon commented Jun 6, 2017

zinoviev commented Jun 6, 2017 • edited Loading

gaearon commented Jun 6, 2017 • edited Loading

zinoviev commented Jun 8, 2017

gaearon commented Jun 29, 2017

zinoviev commented Jul 9, 2017

gaearon commented Jul 9, 2017

zinoviev commented Sep 26, 2017

dbenson24 commented May 24, 2018

inoyakaigor commented Jun 28, 2018

Vannevelj commented Jan 16, 2019

bvaughn commented Aug 20, 2019

zinoviev commented Mar 3, 2017 •

edited

Loading

zinoviev commented Mar 15, 2017 •

edited

Loading

zinoviev Mar 15, 2017 •

edited

Loading

zinoviev Apr 20, 2017 •

edited

Loading

zinoviev commented Jun 6, 2017 •

edited

Loading

gaearon commented Jun 6, 2017 •

edited

Loading