Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve proxies #1053

Merged
merged 8 commits into from
Nov 19, 2019
Merged
Show file tree
Hide file tree
Changes from all 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
10 changes: 10 additions & 0 deletions ember_debug/object-inspector.js
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,13 @@ export default EmberObject.extend(PortMixin, {
*/
mixinDetailsForObject(object) {
const mixins = [];
if (object instanceof Ember.ObjectProxy && object.content && !object._showProxyDetails) {
return this.mixinDetailsForObject(object.content);
}

if (object instanceof Ember.ArrayProxy && object.content && !object._showProxyDetails) {
return this.mixinDetailsForObject(object.slice(0, 101));
Copy link
Member

Choose a reason for hiding this comment

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

What is this slice for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it does the same as toArray(), so it obtains the correct items from e.g. ember data many-array but only for the first 101 items, (though this should actually be part of the perf PR). otherwise it would call objectAt for all items in many-array

}

const own = ownMixins(object);

Expand Down Expand Up @@ -1095,6 +1102,9 @@ function getDebugInfo(object) {
let debugInfo = null;
let objectDebugInfo = get(object, '_debugInfo');
if (objectDebugInfo && typeof objectDebugInfo === 'function') {
if (object instanceof Ember.ObjectProxy && object.content) {
object = object.content;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if maybe we should use Ember.get, wouldn't that resolve proxy values as well as normal objects?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but the new ember data debugInfo does not do it...
always fails here:
https://github.com/emberjs/data/blob/master/packages/model/addon/-private/model.js#L1186

Copy link
Member

Choose a reason for hiding this comment

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

@patricklx can you elaborate on that? Perhaps @runspired can help?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean we use Ember.get here get(object, '_debugInfo')
and it will get through the proxy to the _debugInfo. (in case of accessing belongsTo relation)
but then we bind the _debugInfo to the proxy itself.
So, this.eachAttribute will not be there (or should be accessed with Ember.get, if that works to get functions too?) in https://github.com/emberjs/data/blob/master/packages/model/addon/-private/model.js#L1186
Or _debugInfo needs to check if a proxy is beeing used.

Choose a reason for hiding this comment

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

but the new ember data debugInfo

I wouldn't call this "new", its been around a long time and it's something we'd love to delete :P

Reaching through the proxy for the content seems wise here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mmm, right...
but I think it did not happen before... maybe because im using class extends Model {...} instead of Model.extend(...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, same issue, so might have never worked with belongTo relations :)

}
// We have to bind to object here to make sure the `this` context is correct inside _debugInfo when we call it
debugInfo = objectDebugInfo.bind(object)();
}
Expand Down
132 changes: 131 additions & 1 deletion tests/ember_debug/object-inspector-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ import { inspect } from '@ember/debug';
import { run } from '@ember/runloop';
import { guidFor } from '@ember/object/internals';
import EmberObject, { computed } from '@ember/object';
import MutableArray from '@ember/array/mutable';
import ArrayProxy from '@ember/array/proxy';
import ObjectProxy from '@ember/object/proxy';
import Service from '@ember/service';
import { VERSION } from '@ember/version';
import { tracked } from '@glimmer/tracking';
Expand Down Expand Up @@ -271,6 +274,60 @@ module('Ember Debug - Object Inspector', function(hooks) {
});
});

test('Proxies are skipped by default', function (assert) {
let inspected = EmberObject.extend({
test: 'a'
}).create();
const proxy = ObjectProxy.create({
content: inspected
});
objectInspector.sendObject(proxy);
let computedProperty = message.details[1].properties[0];
assert.equal(computedProperty.name, 'test');
assert.equal(computedProperty.value.inspect, inspect('a'));
});

test('Object Proxies are not skipped with _showProxyDetails', function (assert) {
let inspected = EmberObject.extend({
test: 'a'
}).create();
const proxy = ObjectProxy.create({
content: inspected,
_showProxyDetails: true,
prop: 'b'
});
objectInspector.sendObject(proxy);
let computedProperty = message.details[0].properties[1];
assert.equal(computedProperty.name, 'prop');
assert.equal(computedProperty.value.inspect, inspect('b'));
});

test('Array Proxies show content from toArray', function (assert) {
// support ArrayProxy -> MutableArray for ember data many-array
// https://api.emberjs.com/ember-data/release/classes/ManyArray
// https://github.com/emberjs/data/blob/master/packages/store/addon/-private/system/promise-proxies.js#L130
const array = EmberObject.extend(MutableArray, {
length: 1,
content: ['internal'],
objectAt() {
return 1;
}
}).create();

const proxy = ArrayProxy.create({
content: array
});
objectInspector.sendObject(proxy);

let property = message.details[0].properties[0];
assert.equal(property.name, 0);
assert.equal(property.value.inspect, 1);

property = message.details[0].properties[1];
assert.equal(property.name, 'length');
assert.equal(property.value.inspect, 1);
});

test('Correct mixin properties', function (assert) {

// eslint-disable-next-line ember/no-new-mixins
Expand Down Expand Up @@ -311,7 +368,6 @@ module('Ember Debug - Object Inspector', function(hooks) {
assert.equal(details[3].properties[0].value.inspect, inspect('custom2'));
});


test('Computed properties are correctly calculated', function(assert) {
let inspected = EmberObject.extend({
hi: computed(function() {
Expand Down Expand Up @@ -570,6 +626,80 @@ module('Ember Debug - Object Inspector', function(hooks) {
assert.ok(message.details[3].name !== 'MixinToSkip', 'Correctly skips mixins');
});

test('Property grouping can be customized using _debugInfo when using Proxy', function(assert) {

class MyMixin extends Mixin {
toString() {
return 'MixinToSkip';
}
}

let mixinToSkip = MyMixin.create({});

let Inspected = EmberObject.extend(mixinToSkip, {
name: 'Teddy',
gender: 'Male',
hasChildren: false,
toString: function() {
return 'TestObject';
},
expensiveProperty: computed(function() { return ''; }),
_debugInfo() {
return {
propertyInfo: {
includeOtherProperties: true,
skipProperties: ['propertyToSkip'],
skipMixins: ['MixinToSkip'],
expensiveProperties: ['expensiveProperty'],
groups: [
{
name: 'Basic Info',
properties: ['name', 'gender'],
expand: true
},
{
name: 'Family Info',
properties: ['maritalStatus']
}
]
}
};
}
});

let inspected = Inspected.create({
maritalStatus: 'Single',
propertyToSkip: null
});

const proxy = ObjectProxy.create({
content: inspected
});

objectInspector.sendObject(proxy);

assert.ok(message.name.includes('(unknown)'));

assert.equal(message.details[0].name, 'Basic Info');
assert.equal(message.details[0].properties[0].name, 'name');
assert.equal(message.details[0].properties[1].name, 'gender');
assert.ok(message.details[0].expand);

assert.equal(message.details[1].name, 'Family Info');
assert.equal(message.details[1].properties[0].name, 'maritalStatus');

assert.equal(message.details[2].name, 'Own Properties');
assert.equal(message.details[2].properties.length, 0, 'Correctly skips properties');

assert.equal(message.details[3].name, 'TestObject');
assert.equal(message.details[3].properties.length, 3, 'Correctly merges properties');
assert.equal(message.details[3].properties[1].name, 'hasChildren');
assert.equal(message.details[3].properties[2].name, 'expensiveProperty', 'property name is correct');
assert.equal(message.details[3].properties[2].value.isCalculated, undefined, 'Does not calculate expensive properties');

assert.ok(message.details[3].name !== 'MixinToSkip', 'Correctly skips mixins');
});


test('Service should be successfully tagged as service on serialization', function(assert) {
let inspectedService = Service.extend({
Expand Down