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.

Sign up for GitHub

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

Jump to bottom

Directly reloading a hasMany with links should trigger only one request #2384

Merged
merged 1 commit into from
Dec 7, 2015
Merged

Directly reloading a hasMany with links should trigger only one request #2384

merged 1 commit into from
Dec 7, 2015

Conversation

sly7-7
Copy link
Contributor

@sly7-7 sly7-7 commented Oct 14, 2014

Fixes #2379

cc/ @jcbvm @igorT

igorT
igorT reviewed Oct 16, 2014
View reviewed changes
packages/ember-data/lib/system/record_arrays/many_array.js
@@ -114,6 +117,9 @@ export default RecordArray.extend({
@public
*/
reload: function() {
if (this._loadingPromise) {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this should be handled in the relationship, as we already keep lots of loading data there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was considering the many array like a model, and a model has the same kind of mechanism.
It's handled both in the states, and in the store. I felt like I should do the same for the many array, and I must admit that it was the easiest way.

@sly7-7
Copy link
Contributor Author

sly7-7 commented Oct 17, 2014

@igorT Let me now what do you think about it, since I also reset the isLoaded state when reloading. (It was not the case before this commit, and one assert failed)

@pavloo
Copy link

pavloo commented Jan 25, 2015

any expectations on when this can be merged ?

@sly7-7
Copy link
Contributor Author

sly7-7 commented Jan 26, 2015

@sol1dus I have no idea. @igorT I will gladly rebase if needed 😄

@sly7-7
Copy link
Contributor Author

sly7-7 commented Mar 13, 2015

@igorT I'm sorry to ping you again, but I don't like to keep WIP open too long. Is this behavior still wanted ?

@bmac bmac modified the milestone: 1.0.0-beta.16 Mar 16, 2015
@ghost
Copy link

ghost commented Apr 8, 2015

@sly7-7 curious on the status of this, thanks!

@sly7-7
Copy link
Contributor Author

sly7-7 commented Apr 9, 2015

@bmac Is this something to be discussed during a core meeting ?

@wecc wecc removed the team-review label Apr 24, 2015
@wecc
Copy link
Contributor

wecc commented Apr 24, 2015

There's a rebase branch of this over at https://github.com/emberjs/data/tree/rebase-2384. We need to update some of the tests and have a better way of inspecting the state of promises to know if it's fulfilled or not to land this.

@sly7-7
Copy link
Contributor Author

sly7-7 commented Apr 24, 2015

@wecc thanks for keeping that alive :) I'm currently working on the rebase branch, trying to make the tests pass.

@sly7-7
Copy link
Contributor Author

sly7-7 commented Apr 24, 2015

@wecc I just updated this against the rebase branch, let me know if you think this is this kind of implementation you were waiting for :)

@alisdair
Copy link

It would be great to see this PR progress. I've rebased against master here: https://github.com/alisdair/data/tree/pr/2384

It's not clear from the title, but this also seems to fix the case where a hasMany with link fails to load, and reload() is called to try again. At the moment, if a hasMany with link fails to load, there's nothing that can be done to rectify the situation, as far as I can tell.

@sly7-7
Copy link
Contributor Author

sly7-7 commented Jun 30, 2015

@wecc Was that discussed during the meetings ?

@wecc
Copy link
Contributor

wecc commented Jun 30, 2015

@sly7-7 I do not remember fully tbh. I think @igorT and @stefanpenner had a discussion regarding how to inspect the state of a promise but I'm unsure what was decided.

@bmac
Copy link
Member

bmac commented Oct 12, 2015

@wecc or @sly7-7 what is the status on this pr. Is there a decision/insight needed from @igorT or @stefanpenner?

@sly7-7
Copy link
Contributor Author

sly7-7 commented Oct 13, 2015

@bmac, I think the point here is https://github.com/emberjs/data/pull/2384/files#diff-0c8d73828c8e0096b8bf3d5ef10cac5aR105, where we inspect if the promise is still pending or not.
For me I just use the promise api, so it ok, but there maybe something more subtle to my eyes, so yes, if Igor or Stefan could give us some piece of their brain here it would be great 😄

stefanpenner
stefanpenner reviewed Oct 13, 2015
View reviewed changes
packages/ember-data/tests/integration/relationships/has-many-test.js
@@ -450,6 +450,81 @@ test("A hasMany relationship can be reloaded if it was fetched via ids", functio
});
});

test("A hasMany relationship can be reloaded even if it failed at the first time", function() {
Copy link
Member

Choose a reason for hiding this comment

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

"even if it failed at the first time" => "even if it failed the first time"

@bmac
Copy link
Member

bmac commented Oct 13, 2015

Thanks @sly7-7. I got @stefanpenner to look at that this morning and he said it looks good. Do you mind rebasing this pr?

stefanpenner
stefanpenner reviewed Oct 13, 2015
View reviewed changes
packages/ember-data/tests/integration/relationships/has-many-test.js
}
};
run(function() {
env.store.find('post', 1).then(async(function(post) {
Copy link
Member

Choose a reason for hiding this comment

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

do we actually need these asyncs? qunit supports promises, we merely need to return the promise chain. This will also improve error scenarios in the test suite..

@stefanpenner
Copy link
Member

I suspect on completion (in this case rejection) the _loadingPromise could just be removed, so future reloads merely start fresh.

@sly7-7
Copy link
Contributor Author

sly7-7 commented Oct 13, 2015

Technically, that means doing something like this._loadingPromise = null in the rejected case ?

@stefanpenner
Copy link
Member

Technically, that means doing something like this._loadingPromise = null in the rejected case ?
Show all checks

ya, potentially also on any completion.

@sly7-7
Copy link
Contributor Author

sly7-7 commented Oct 13, 2015

Ok, thanks for the review @stefanpenner 😄
@bmac I will try to update and rebase this afternoon. If I can't, do you want to get this merged before the next release ?

@bmac
Copy link
Member

bmac commented Oct 13, 2015

Thanks @sly7-7. I'm in no runs to get anything in before the next release, so take your time.

@sly7-7
Copy link
Contributor Author

sly7-7 commented Oct 15, 2015

@bmac Done 😄
@igorT Would mind taking a look and confirm this could be squashed-and-merged ?

@bmac
Copy link
Member

bmac commented Dec 4, 2015

@sly7-7 Do you have time to rebase and squash this pr?

@sly7-7
Copy link
Contributor Author

sly7-7 commented Dec 4, 2015

@bmac sure, I will do it tonight :)

@sly7-7
Directly reloading a hasMany with links should trigger only one request
94fd642 
Introducing a loading promise in the many relationship
in order to keep track of its loading state.
@sly7-7
Copy link
Contributor Author

sly7-7 commented Dec 4, 2015

@bmac Done 😄

bmac added a commit that referenced this pull request Dec 7, 2015
@bmac
Merge pull request #2384 from sly7-7/reload-has-many-not-yet-fetched-…
f1485cb 
…with-links

Directly reloading a hasMany with links should trigger only one request
@bmac bmac merged commit f1485cb into emberjs:master Dec 7, 2015
@bmac
Copy link
Member

bmac commented Dec 7, 2015

Thanks @sly7-7

@sly7-7 sly7-7 deleted the reload-has-many-not-yet-fetched-with-links branch December 7, 2015 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Linked hasMany relationship gets loaded twice when reloading
7 participants
@sly7-7 @pavloo @wecc @alisdair @bmac @stefanpenner @igorT