Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Deferred object getters evaluation #569

Closed
wants to merge 15 commits into from
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion agent/Agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ import type Bridge from './Bridge';
* - mount
* - update
* - unmount
* - sendGetterValue
*/
class Agent extends EventEmitter {
// the window or global -> used to "make a value available in the console"
Expand Down Expand Up @@ -153,6 +154,7 @@ class Agent extends EventEmitter {
bridge.on('setState', this._setState.bind(this));
bridge.on('setProps', this._setProps.bind(this));
bridge.on('setContext', this._setContext.bind(this));
bridge.on('evalGetter', this._evalGetter.bind(this));
bridge.on('makeGlobal', this._makeGlobal.bind(this));
bridge.on('highlight', id => this.highlight(id));
bridge.on('highlightMany', id => this.highlightMany(id));
Expand Down Expand Up @@ -216,6 +218,7 @@ class Agent extends EventEmitter {
bridge.forget(id);
});
this.on('setSelection', data => bridge.send('select', data));
this.on('sendGetterValue', (data) => bridge.send('evalgetterresult', data));
this.on('setInspectEnabled', data => bridge.send('setInspectEnabled', data));
}

Expand Down Expand Up @@ -334,6 +337,16 @@ class Agent extends EventEmitter {
}
}

_evalGetter({id, path}: {id: ElementID, path: Array<string>}) {
var data = this.elementData.get(id);
if (!data) {
return;
}
var obj = data.props;
var value = path.reduce((obj_, attr) => obj_ ? obj_[attr] : null, obj);
this.emit('sendGetterValue', {id, path, value});
}

_makeGlobal({id, path}: {id: ElementID, path: Array<string>}) {
var data = this.elementData.get(id);
if (!data) {
Expand Down Expand Up @@ -445,7 +458,7 @@ class Agent extends EventEmitter {
if (!id) {
return;
}

this.highlight(id);
}
}
Expand Down
17 changes: 16 additions & 1 deletion agent/dehydrate.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,15 @@ function dehydrate(data: Object, cleaned: Array<Array<string>>, path?: Array<str

var res = {};
for (var name in data) {
res[name] = dehydrate(data[name], cleaned, path.concat([name]), level + 1);
if (isPropertyGetter(data, name)) {
cleaned.push(path.concat([name]));
res[name] = {
name: name,
type: 'getter',
};
} else {
res[name] = dehydrate(data[name], cleaned, path.concat([name]), level + 1);
}
}
return res;
}
Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My fail, will fix asap!

return descriptor.get !== undefined;
/* eslint-enable no-proto */
}

module.exports = dehydrate;
12 changes: 12 additions & 0 deletions backend/__tests__/copyWithSet-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,16 @@ describe('copyWithSet', function() {
var res = copyWithSet([0, 1, {2: {3: [4, 5, {6: {7: 8}}, 9], 10: 11}}, 12], [2, '2', '3', 2, '6', '7'], 'moose');
expect(res).toEqual([0, 1, {2: {3: [4, 5, {6: {7: 'moose'}}, 9], 10: 11}}, 12]);
});

it('must copy descriptors', function() {
var obj = {
_name: 'foo',
get name() {
return this._name + 'bar';
},
};

var res = copyWithSet(obj, ['a'], 'b');
expect(res.name).toEqual('foobar');
});
});
18 changes: 17 additions & 1 deletion backend/copyWithSet.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ function copyWithSetImpl(obj, path, idx, value) {
return value;
}
var key = path[idx];
var updated = Array.isArray(obj) ? obj.slice() : {...obj};
var updated = Array.isArray(obj) ? obj.slice() : assignWithDescriptors(obj);
// $FlowFixMe number or string is fine here
updated[key] = copyWithSetImpl(obj[key], path, idx + 1, value);
return updated;
Expand All @@ -25,4 +25,20 @@ function copyWithSet(obj: Object | Array<any>, path: Array<string | number>, val
return copyWithSetImpl(obj, path, 0, value);
}

function assignWithDescriptors(source) {
/* eslint-disable no-proto */
var target = Object.create(source.__proto__);
/* eslint-enable no-proto */

Object.defineProperties(target, Object.keys(source).reduce((descriptors, key) => {
var descriptor = Object.getOwnPropertyDescriptor(source, key);
if (descriptor.hasOwnProperty('writable')) {
descriptor.writable = true;
}
descriptors[key] = descriptor;
return descriptors;
}, {}));
return target;
}

module.exports = copyWithSet;
4 changes: 4 additions & 0 deletions frontend/DataView/DataView.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import type {Theme, DOMEvent} from '../types';
var {sansSerif} = require('../Themes/Fonts');
var React = require('react');
var Simple = require('./Simple');
var Getter = require('./Getter');

var consts = require('../../agent/consts');
var previewComplex = require('./previewComplex');
Expand Down Expand Up @@ -214,6 +215,9 @@ class DataItem extends React.Component {
/>
);
complex = false;
} else if (data[consts.type] === 'getter') {
preview = <Getter data={data} path={this.props.path} />;
complex = false;
} else {
preview = previewComplex(data, theme);
}
Expand Down
41 changes: 41 additions & 0 deletions frontend/DataView/Getter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @flow
*/
'use strict';
var React = require('react');

class Getter extends React.Component {
handleClick() {
this.context.onEvalGetter(this.props.path);
}

constructor(props: Object) {
super(props);
}

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

const style = {
'cursor': 'pointer',
};

Getter.propTypes = {
data: React.PropTypes.any,
path: React.PropTypes.array,
};

Getter.contextTypes = {
onEvalGetter: React.PropTypes.func,
};

module.exports = Getter;
7 changes: 7 additions & 0 deletions frontend/PropState.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ class PropState extends React.Component {
onChange: (path, val) => {
this.props.onChange(path, val);
},
onEvalGetter: (path) => {
this.props.onEvalGetter(path);
},
};
}

Expand Down Expand Up @@ -176,6 +179,7 @@ PropState.contextTypes = {

PropState.childContextTypes = {
onChange: React.PropTypes.func,
onEvalGetter: React.PropTypes.func,
};

var WrappedPropState = decorate({
Expand Down Expand Up @@ -206,6 +210,9 @@ var WrappedPropState = decorate({
showMenu(e, val, path, name) {
store.showContextMenu('attr', e, store.selected, node, val, path, name);
},
onEvalGetter(path) {
store.evalGetter(store.selected, path.slice(1));
},
inspect: store.inspect.bind(store, store.selected),
};
},
Expand Down
18 changes: 12 additions & 6 deletions frontend/PropVal.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,23 +122,29 @@ function previewProp(val: any, nested: boolean, inverted: boolean, theme: Theme)
return <span style={style}>{`${val[consts.name]}[${val[consts.meta].length}]`}</span>;
}
case 'iterator': {
style = {
color: inverted ? getInvertedWeak(theme.state02) : theme.base05,
style = {
color: inverted ? getInvertedWeak(theme.state02) : theme.base05,
};
return <span style={style}>{val[consts.name] + '(…)'}</span>;
}
case 'symbol': {
style = {
color: inverted ? getInvertedWeak(theme.state02) : theme.base05,
style = {
color: inverted ? getInvertedWeak(theme.state02) : theme.base05,
};
// the name is "Symbol(something)"
return <span style={style}>{val[consts.name]}</span>;
}
case 'getter': {
style = {
color: inverted ? getInvertedWeak(theme.state02) : theme.base05,
};
return <span style={style}>(…)</span>;
}
}

if (nested) {
style = {
color: inverted ? getInvertedWeak(theme.state02) : theme.base05,
style = {
color: inverted ? getInvertedWeak(theme.state02) : theme.base05,
};
return <span style={style}>{'{…}'}</span>;
}
Expand Down
17 changes: 17 additions & 0 deletions frontend/Store.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ const DEFAULT_PLACEHOLDER = 'Search (text or /regex/)';
* - toggleCollapse
* - toggleAllChildrenNodes
* - setProps/State/Context
* - evalGetter
* - makeGlobal(id, path)
* - setHover(id, isHovered, isBottomTag)
* - selectTop(id)
Expand Down Expand Up @@ -176,6 +177,7 @@ class Store extends EventEmitter {
this._bridge.on('mount', (data) => this._mountComponent(data));
this._bridge.on('update', (data) => this._updateComponent(data));
this._bridge.on('unmount', id => this._unmountComponent(id));
this._bridge.on('evalgetterresult', (data) => this._setGetterValue(data));
this._bridge.on('setInspectEnabled', (data) => this.setInspectEnabled(data));
this._bridge.on('select', ({id, quiet}) => {
this._revealDeep(id);
Expand Down Expand Up @@ -423,6 +425,10 @@ class Store extends EventEmitter {
this._bridge.send('setContext', {id, path, value});
}

evalGetter(id: ElementID, path: Array<string>) {
this._bridge.send('evalGetter', {id, path});
}

makeGlobal(id: ElementID, path: Array<string>) {
this._bridge.send('makeGlobal', {id, path});
}
Expand Down Expand Up @@ -698,6 +704,17 @@ class Store extends EventEmitter {
this._nodesByName = this._nodesByName.set(node.get('name'), this._nodesByName.get(node.get('name')).delete(id));
}
}

_setGetterValue(data: DataType) {
var node = this._nodes.get(data.id);
var props = node.get('props');
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);
Copy link
Contributor Author

@zinoviev zinoviev Mar 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Props must be new object or bounded Node wont rerenders

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@zinoviev zinoviev Apr 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining, this makes sense.

this._nodes = this._nodes.setIn([data.id, 'props'], newProps);
this.emit(data.id);
}
}

module.exports = Store;
22 changes: 22 additions & 0 deletions test/example/target.js
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,27 @@ for (var mCount = 200; mCount--;) {
}


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

var getterOnProtoProp = Object.create(protoWithGetter);
getterOnProtoProp.name = 'Foo';

let setterTestProp = {
_value: 'hello',
_editsCounter: 0,
get lol() {
return this._value.toUpperCase();
},
set lol(val) {
this._editsCounter ++;
this._value = val;
},
};

class Wrap extends React.Component {
render() {
return (
Expand All @@ -421,6 +442,7 @@ class Wrap extends React.Component {
<PropTester {...complexProps}/>
<PropTester {...uninspectableProps}/>
<PropTester massiveMap={massiveMap}/>
<PropTester withGetter={getterOnProtoProp} withSetter={setterTestProp}/>
</div>
);
}
Expand Down