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

first request to findAll does not resolve correctly #1013

Closed
dagda1 opened this issue May 31, 2013 · 26 comments
Closed

first request to findAll does not resolve correctly #1013

dagda1 opened this issue May 31, 2013 · 26 comments

Comments

@dagda1
Copy link

dagda1 commented May 31, 2013

I am using the latest from master (revision 13)

I am finding that the first request to any resource returns 0 records on the first call to findAll, for example

App.Contact.find().then (contacts) ->
  console.log contacts.get('length')

What I am finding is that the console.log statement above will log 0 but if I then type:

App.Contact.find().get('length')

into the console after the page has refreshed and a call to findAll has been made, the length is as I would expect, i.e. greater than 0.

This happens for any resource, I have stepped through the code and the json is getting materialized into the models but for some reason, the record array is empty.

I have stepped through the code and it appears that the promise is resolving before didFIndAll is called.

I cannot create a fiddle for this as it is only happening with the rest adapter.

@sly7-7
Copy link
Contributor

sly7-7 commented May 31, 2013

@dagda1 Actually, DS.Model.find() return a 'promistified' RecordArray whch promise is resolved when the recordarray is loaded (ie its isLoaded propery is true). The problem is that DS.Model.find() is loaded as soon as the array is created, as a consequence the promise is resolved.

In order to fix your problem (because I think you want to return a live array), something like this should work:

App.Contact.find({}).then(contacts) ->
   return App.Contact.all()

This way, under the hood, you are calling a findQuery, which is loaded when the backend returns the records, not at the array creation.

cc/ @tchak, I'd like you to confirm this though, to be sure I'm right.

@ahawkins
Copy link
Contributor

@sly7-7 this sounds like the perfect amount of hax to be correct. This would not surprise me at all.

@dagda1
Copy link
Author

dagda1 commented May 31, 2013

That does indeed work.

@sly7-7
Copy link
Contributor

sly7-7 commented May 31, 2013

Yeah, so I would say it's not an "issue" because this is the way it's intended to work. But this should definitely change. I really hope @tchak and @stefanpenner could work together in order to make ember-data and promises working great together.

@dagda1
Copy link
Author

dagda1 commented May 31, 2013

I think the current behaviour of find will be unexpected to most people. It was to me anyway.

@ahawkins
Copy link
Contributor

Confirm. The current behavior makes no sense.

On Fri, May 31, 2013 at 11:08 PM, Paul notifications@github.com wrote:

I think the current behaviour of find will be unexpected to most people.
It was to me anyway.


Reply to this email directly or view it on GitHubhttps://github.com//issues/1013#issuecomment-18771688
.

@sly7-7
Copy link
Contributor

sly7-7 commented May 31, 2013

@dagda1 @ahawkins I agree with you, but as explained here #735 (comment) we have to define when should we consider the array as "loaded".
In ths specific case of resolving the promise for the findAll (not the all() one), perhaps we could resolve it when the isUpdating flag is set to false.

@shonsaoji
Copy link

I faced the same issue.

I am using ruby on rails with include_root_in_json set to true..
The response format for API /verticals is

{
verticals:[{
verticals: {id: 1, name: vertical_one},
verticals: {id: 2, name: vertical_two},
}]
};
To handle this I had to change the following -
line: 7081
method: extractMany
file: ember-data.js

var reference = this.extractRecordRepresentation(loader, type, objects[i]); // <-- changed this to pass objects[i][root] to the method.

That fixed it for me.

@towerhe
Copy link

towerhe commented Jun 3, 2013

@shonsaoji I think you should response:

{
  verticals: [{
    id: 1, name: "vertical_one"
  }, {
    id: 2, name: "vertical_two"
  }]
}

You could use active_model_serializers to serialize your models.

@tchak
Copy link
Member

tchak commented Jun 3, 2013

@dagda1 @ahawkins @sly7-7 what happens if you create a record on the client before calling find? And you know that RecordArray returned from find is the same that the one returned from all right?
Good news, I think I may have an idea :) You focussing on find, where what you want is a more flexible filter.
You may already accomplish almost what you want by calling :

App.Post.filter({}, Em.K);

The only part missing is isLoaded flag. I think it would not be unreasonable to set it to true only on adapter response in case filter is called with two arguments. Patch with a proposition is in progress :)

@tchak
Copy link
Member

tchak commented Jun 3, 2013

@dagda1 @ahawkins would you be happy with #1015 ?

App.Post.filter({}, Em.K).then(function() { console.log("Tada!"); });

@dagda1
Copy link
Author

dagda1 commented Jun 3, 2013

My concern would be that people new to ember (which we need to encourage more of) would not know what to make of:

App.Post.filter({}, Em.K)

Maybe we should give it some sugar in the form of a nicer name that delegates to this?

findAll would be my choice (joke) :)

@tchak
Copy link
Member

tchak commented Jun 3, 2013

@dagda1 People new to ember should not need it :)

@jaaksarv
Copy link
Contributor

This is definitely a bug. Maybe not implementation bug (if it is intented to be so), but design bug. isLoaded should become true only when finished loading data from the server. If there is a need in Ember internal workings to observer when the "live array" becomes initialised then please implement a isArrayInitilised state for this. Or please at least add isReallyReallyLoaded state so we can build applications.

find({}) workaround to trigger findQuery is not a solution, but a hack. It does not work for example for FixtureAdapter.

@dagda1
Copy link
Author

dagda1 commented Jun 12, 2013

@jaaksarv I have to agree. IsLoaded should not be ambiguous. You would only ever expect that isLoaded would equal true if it had finished loading data from the server.

@tchak
Copy link
Member

tchak commented Jun 12, 2013

@jaaksarv @dagda1 and what should happen in case you have no server?

@dagda1
Copy link
Author

dagda1 commented Jun 12, 2013

If you have no server then you need to somehow set the isLoaded to true initially but it should not set out with a default of true.

@tchak
Copy link
Member

tchak commented Jun 12, 2013

Why you don't use isUpdating instead of isLoaded? The promise issue is a another one. In the perfect world both should be de decorrelated.

@jaaksarv
Copy link
Contributor

@tchak If you have no server then isLoaded should become true once the array is populated with data. I'm currently using FixtureAdapter for development before connecting my application to the server. FixtureAdapter has some dalay in it to emulate asynchronous server communication behavior. And with FixtureAdapter the isLoaded property also becomes true before the array is filled with data.

@jaaksarv
Copy link
Contributor

@tchak isUpdating does not help. isUpdating flag will become true and then false, both before the array is populated.

I also went through ember-data code (v0.13). There are no comments on isUpdating property on DS.RecordArray so hard to tell what it is and how it should behave.

From the code I can tell that store.didUpdateAll will set isUpdating to false, but this will happen before serializer.extractMany is called.

Anyway I think that isLoaded should be fixed for findAll. isLoaded should have the same meaning whatever loading type is used (find single record, findAll, findQuery, findMany). Current behavior is confusing. If there is need to have different logic for "live array" then some other property should be used for this and not isLoaded.

@tchak
Copy link
Member

tchak commented Jun 13, 2013

@jaaksarv you right about isUpdating. I fixed it in this pr #1030. It needs to be better documented. We also need to return a promise from the update() method.

The main issue we have now is the fact that records/record arrays are promises them selves. They are resolved on "load" and stay this way. It need to change. The confusion is related to the fact tat isLoaded is not related to the content but to the record array itself. So in case of "live array" it is alway loaded. But it content may not be. find() / update() should return a promise wich would be resolved with new data loaded from server. But isLoaded flag will always be true in case of "live array".

In sproutcore data store the concept of "live query" and "remote query" were more separated. I agree that by providing a more unified api we introduced some confusion. I think by solving the promises situation and with better documentation we should be able solve all the use cases.

@nadeemyasin61
Copy link

@tchak : i have a problem here.
I m using the following code

    App.User.find({}).then(function(users) {
        all = App.User.all()
        console.log(all)
    });

And here is my json

{
  users: [{
    name: "Nadeem",
    comments: [{
        id: 1
      },
      {
        id: 2
      }]
  }]
}

But when i console "all", it just shows a class object. Something like this.
Class
ember1371577118896: "ember297"
__ember1371577118896_meta: Meta
_super: undefined
get content: function () {
isLoaded: true
isUpdating: false
set content: function (value) {
store: Class
toString: function () { return ret; }
type: App.User
__proto
: Object

even i have tried this: console.log(all.get('users'))

@tchak
Copy link
Member

tchak commented Jun 18, 2013

@nadeemyasin61 what you expect to see? You got a RecordArray from all(). You can think of it as a subclass of an array. You can call all of the array methods on it like forEach or use it directly in your views.

@nadeemyasin61
Copy link

@tchak thanks man. I was trying to use like a regular JS array with []. Make it to work. 👍

@wagenet
Copy link
Member

wagenet commented Aug 13, 2013

We'll be changing the way promises work soon, which should in turn resolve this.

@wagenet wagenet closed this as completed Aug 13, 2013
@wagenet
Copy link
Member

wagenet commented Aug 13, 2013

I should have said "changing the way we use promises". We will not be changing the actual behavior of promises themselves :)

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

9 participants