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

Improve proxies #1053

merged 8 commits into from
Nov 19, 2019

Conversation

patricklx
Copy link
Collaborator

No description provided.

}

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

@@ -1037,6 +1044,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 :)

Copy link
Member

@RobbieTheWagner RobbieTheWagner left a comment

Choose a reason for hiding this comment

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

@patricklx this looks good to me! Can you rebase and fix the conflicts please?

@RobbieTheWagner
Copy link
Member

@patricklx I fixed the conflicts, but seems like we have some test failures now.

@RobbieTheWagner RobbieTheWagner merged commit 7a561c4 into emberjs:master Nov 19, 2019
nummi pushed a commit to nummi/ember-inspector that referenced this pull request Apr 1, 2020
* improve proxies

* fix test
nummi pushed a commit to nummi/ember-inspector that referenced this pull request Apr 5, 2020
* improve proxies

* fix test
patricklx added a commit to patricklx/ember-inspector that referenced this pull request Sep 19, 2022
* improve proxies

* fix test
@patricklx patricklx deleted the fix-proxy branch October 22, 2022 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants