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

Pushing to an arrayProxy triggers calls to objectAtContent on the content array #5379

Closed
igorT opened this issue Aug 15, 2014 · 19 comments
Closed

Comments

@igorT
Copy link
Member

igorT commented Aug 15, 2014

If you have two arrayProxies, pushing to the top most proxy triggers calls to objectAtContent on the child proxy:
http://emberjs.jsbin.com/samexo/1/edit

Seems like pushing objects shouldn't cause them to be accessed. In fact, only the first and last objects get accessed, due to this check:
https://github.com/emberjs/ember.js/blob/master/packages/ember-runtime/lib/mixins/array.js#L425

@stefanpenner
Copy link
Member

the proxies are currently very eager. @mmun has some WIP i believe on making them lazy

@igorT
Copy link
Member Author

igorT commented Aug 15, 2014

Seems like we just want to repeat the isWatching check for firstObject and lastObject from https://github.com/emberjs/ember.js/blob/master/packages/ember-runtime/lib/mixins/array.js#L367

@igorT
Copy link
Member Author

igorT commented Aug 15, 2014

Tried that, broke tests, might investigate later. This bug will make lazy hasMany's a bit more annoying because they will eagerly fetch first/last records

@mmun
Copy link
Member

mmun commented Aug 15, 2014

I agree with @igorT.

@mmun
Copy link
Member

mmun commented Aug 15, 2014

The problem is that even if no one is watching you need to clear the cache if this.objectAt(0) !== cachedFirst would have been true, otherwise get(arr, 'firstObject') will not recompute.

@igorT
Copy link
Member Author

igorT commented Aug 15, 2014

Only if the cache has been set though. I think ignoring it if the cache isn't set would cover lots of cases

@mmun
Copy link
Member

mmun commented Aug 15, 2014

Yeah. Alternatively, objectAt could have its own cache which becomes invalid by carefully tracking the array operations on the content. That's basically the lazy approach.

@mmun
Copy link
Member

mmun commented Aug 15, 2014

I implemented your suggestion (link to diff) and the tests pass. It is pretty hacky though.

@igorT
Copy link
Member Author

igorT commented Aug 15, 2014

I'll turn the jsbin into a real test tomorrow

@krisselden
Copy link
Contributor

@mmun we need to make lazyGet a first class thing for alias descriptor and to work with paths. we could also use it here.

I don't like things reaching into meta directly and making assumptions, it makes it hard to change stuff later.

/cc @stefanpenner

@krisselden
Copy link
Contributor

I'm also uncertain that Ember Data doing ajax as a side effect of get() is good design. I can see get() returning a promise, and you having to explicitly load data you want, but I think we maybe over relying on the lazy computation of CPs, and unexpected ajax as a consequence seems a little steep.

@krisselden
Copy link
Contributor

Want to add a reference to #5289 as I think alias should just forward lazyGet() to a Ember.lazyGet(obj, propPath) and for desc to have desc.lazyGet(obj, key), then you can just do lazyGet(content, 'firstObject')

@locks
Copy link
Contributor

locks commented Aug 16, 2014

I'm also uncertain that Ember Data doing ajax as a side effect of get() is good design. this does sound like it might surprise developers.

@igorT
Copy link
Member Author

igorT commented Aug 16, 2014

Thats how it works now. Not sure what you propose as the alternative design

@stefanpenner
Copy link
Member

First step seems like allowing array proxies to be lazier

@locks
Copy link
Contributor

locks commented Aug 16, 2014

I've just applied the diff that @mmun links on #5379 (comment) and it fixed the problem.

[edit] You can check the tests at https://github.com/locks/data/blob/async-has-many/packages/ember-data/tests/integration/relationships/async_has_many_test.js

@wagenet
Copy link
Member

wagenet commented Aug 22, 2014

@mmun: should that patch become a PR?

@opichals
Copy link
Contributor

I have put together I hope a valid PR: #5591.

@wagenet
Copy link
Member

wagenet commented Nov 1, 2014

Closing in favor of the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants