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

Ember store.findAll not getting receiving records in afterModel hook #3657

Closed
webpapaya opened this issue Aug 14, 2015 · 14 comments
Closed

Ember store.findAll not getting receiving records in afterModel hook #3657

webpapaya opened this issue Aug 14, 2015 · 14 comments

Comments

@webpapaya
Copy link

Hey,

I'm using ember-data 1.13.7 and I have the following Route which loads data from the server. And I need to do some computation on multiple models which are not related to each other. I have the following code.

import Ember from 'ember';

export default Ember.Route.extend({
  model() {
    return Ember.RSVP.hash({
      modelA: this.store.findAll('model-a', {
        backgroundReload: false
      }),
      modelB: this.store.find('model-b', {
        backgroundReload: false
      })
    });
  },

  afterModel(model) {
    console.log(model.modelA.get('length')); // 0
    console.log(model.modelB.get('length')); // eg. 12
  }
});

When I'm using the deprecated find() Method the Models are passed in to the store but when using the new findAll method the models are resolved at a later time.

@webpapaya
Copy link
Author

If you use

 this.store.query('model-a', {});

it works as expected. I really think this is a bug.

@NickMele
Copy link

I think this is due to the difference in the promises that query and findAll return. It seems that findAll returns an immediately resolved promise that updates as records are loaded. Whereas query returns a promise that won't resolve until the model is completely loaded. So @webpapaya I think you are correct that rather than findAll you will want to use query.

@dknutsen
Copy link

I was pulling my hair out for hours with this a couple days ago. Thought I had just done something wrong. Ended up with a workaround. I feel like this should be documented better at the very least.

@NickMele
Copy link

Agreed it definitely should be documented better. I had to dig through the source code of findAll and query to figure that out.

@webpapaya
Copy link
Author

Shouldn't all the query methods in ember behave the same? So what's the difference if i query for a record or if somebody uses findAll? Is there a reason why findAll immediatly resolves the promise?
In my opinion findAll, queryAll and findRecord, queryRecord should resolve their promise the same way. Otherwise this leads to confusion.

@NickMele
Copy link

I definitely agree that it leads to confusion and needs to be documented much better, but it is vaguely in the documentation as such.

findAll return a promise that immediately resolves to a DS.AdapterPopulatedRecordArray with a flag that it is currently loading data.

query returns a promise that gets resolved with a DS.RecordArray once the server returns the data.

@btecu
Copy link
Contributor

btecu commented Sep 10, 2015

Ran into this yesterday. Shouldn't findAll for consistency resolve the promise when the records have been loaded?

@alexanderchr
Copy link

Ran into this as well. Is there any reason at all for findAll to behave like this?

@webpapaya
Copy link
Author

In terms of caching the behaviour of findAll and find makes lot's of sense. So according to the ember-data 1.13 release the findAll and find methods are doing the following thing:

  • First time store.find is called, fetch new data
  • Next time return cached data
  • Fetch new data in the background and update

http://emberjs.com/blog/2015/06/18/ember-data-1-13-released.html#toc_better-caching-defaults-for-code-findall-code-and-code-findrecord-code

So under the hood those methods are doing the following:

  • first findAll
    • find records from the store (resolve first promise -> length 0, because no data is in the store)
    • fetch new data in the background (resolves second promise)
  • second findAll
    • find records from the store (resolve first promise with cached data)
    • fetch new data in the background (resolves second promise with new data)

The reason why query works is, that it doesn't ask the store before fetching data. That's why it resolves the promise with the fetched data from the server.

I think it would be better if the first findAll waits for the server to respond, so that the workflow is as following:

  • first findAll
    • fetch new data(resolves promise)
  • second findAll
    • find records from the store (resolve first promise with cached data)
    • fetch new data in the background (resolves second promise with new data)

@dknutsen
Copy link

This just bit me yet again. Part of me likes the behavior the way it is intended but it just isn't clear what is going to happen from the function name/documentation.

EDIT: after reading the 1.13 blog post more carefully I actually find the current behavior ok and actually nice, but it definitely needs to be documented much better in the DS.Store docs.

@Leeft
Copy link

Leeft commented Nov 1, 2015

This has bitten me too, been struggling with it for days now. Aside from the documentation needing work, the biggest problem I have with it is that I haven't yet found a way to make good use of it in code (Ember itself copes just fine). How this now should work is actually much like Firebase does things already with its native API, though there it's well documented how you can first get an "initial set of data" and then "listen for updates".

In my application route I'm preloading several models from Firebase; then in a "display a map" component I need to work with these records and draw them into a WebGL canvas. So far though, both my application route and component are initialized well before the data is actually loaded and I'm struggling to find a way to attach a promise that responds as data is still coming in. I'll have to try some of the suggestions above.

@frederikbosch
Copy link

In my opinion it is very very important that one method always has the same behaviour, so doing the first time something different then other times is weird as suggested by @webpapaya.

However, a user opts in for different behaviour when he chooses to reload in the background. So @webpapaya does have a point here. Moreover the signatures of the shouldBackgroundReloadRecord and shouldBackgroundReloadAll method clearly say reload. There would be only a reload if there were records in the first place. The current behaviour is more like shouldBackgroundLoadAll, so with re omitted.

So I think the suggestions by @webpapaya should be taken into account.

@frederikbosch
Copy link

Oh, I think this bug and #3483 are duplicates.

@pangratz
Copy link
Member

pangratz commented May 4, 2016

@webpapaya the backgroundReload option passed to store.findAll is currently not implemented, but it will be once #4356 is merged.

I am going to close this issue, since the documentation for the reload and background reload behavior should have improved now that #4338 has been merged. Feel free to reopen / comment here if you think this still is not documented sufficiently. Thanks!

@pangratz pangratz closed this as completed May 4, 2016
stoman added a commit to stoman/travelvideo that referenced this issue Nov 13, 2016
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

8 participants