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

pass snapshot through to urlForFindHasMany and urlForFindBelongsTo #4308

Merged
merged 2 commits into from
Apr 7, 2016
Merged

pass snapshot through to urlForFindHasMany and urlForFindBelongsTo #4308

merged 2 commits into from
Apr 7, 2016

Conversation

trevorrjohn
Copy link
Contributor

related to: #4304

@trevorrjohn
Copy link
Contributor Author

Not sure what is going on with appveyor here. I took a look at the logs, but wasn't able to discern the problem.

@trevorrjohn trevorrjohn force-pushed the tj/snapshots-for-finders branch from 79d8bd7 to edf0139 Compare April 7, 2016 14:13
pangratz
pangratz reviewed Apr 7, 2016
View reviewed changes
addon/adapters/rest.js
@@ -1279,7 +1279,7 @@ if (isEnabled('ds-improved-ajax')) {

case 'findHasMany':
case 'findBelongsTo':
let url = this.buildURL(type.modelName, id, null, requestType);
let url = this.buildURL(type.modelName, id, snapshots, requestType);
Copy link
Member

Choose a reason for hiding this comment

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

We want to pass the snapshot here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I am still confused about when snapshot vs snapshots is passed in here.

Is snapshot always a DS.Snapshot and this snapshots always DS.SnapshotRecordArray?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Basically the snapshot here represents the "owner" of the relationship.

@pangratz
Copy link
Member

pangratz commented Apr 7, 2016

You also need to pass the snapshot as third argument here and here. Since this hasn't been caught a test, can you modify tests for the findBelongsTo and findHasMany. I guess an assertion that the passed snapshot is present or it is an instance of DS.Snapshot would be good.

Not sure what is going on with appveyor here. I took a look at the logs, but wasn't able to discern the problem.

Don't worry too much about that, unfortunately AppVeyor fails sporadically. Restarting the CI by a force push helps most of the time...

Apart from my comments, this looks great!

@trevorrjohn trevorrjohn force-pushed the tj/snapshots-for-finders branch from edf0139 to b18a4c0 Compare April 7, 2016 17:40
@trevorrjohn
Copy link
Contributor Author

So I updated the docs and added the snapshot where you pointed to above. I think I am still a little lost on what is going on between the call to findRecord and the call to findHasMany. I did some digging, but wasn't really able to follow the code path of where findRecord starts loading the associations.

pangratz
pangratz reviewed Apr 7, 2016
View reviewed changes
addon/adapters/rest.js
@@ -1154,6 +1154,10 @@ var RESTAdapter = Adapter.extend(BuildURLMixin, {
buildQuery(snapshot) {
let query = {};

if (snapshot) {
query.adapterOptions = snapshot.adapterOptions
}
Copy link
Member

Choose a reason for hiding this comment

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

This isn't needed, the main thing this PR tries to solve is just passing the snapshot to the urlForFindBelongsTo and urlForFindHasMany hook. Sorry if my comments confused you here...

@pangratz
Copy link
Member

pangratz commented Apr 7, 2016

I think I am still a little lost on what is going on between the call to findRecord and the call to findHasMany. I did some digging, but wasn't really able to follow the code path of where findRecord starts loading the associations.

The urlForFindBelongsTo and urlForFindHasMany hooks are not called as a result of a store.findXXX or something, but as the result of a linked relationship: it is possible that a relationship in ember-data is defined not via an id but via a link (for example defined here in the test). The urlForFindBelongsTo hook is then called as a result of accessing the linked relationship here...

@trevorrjohn trevorrjohn force-pushed the tj/snapshots-for-finders branch from 359fc21 to 85a1220 Compare April 7, 2016 18:37
@trevorrjohn
Copy link
Contributor Author

ok updated, thanks for walking me through that. I was lost there for a bit.

@pangratz pangratz merged commit 0a485da into emberjs:master Apr 7, 2016
@pangratz
Copy link
Member

pangratz commented Apr 7, 2016

Yeah, this can be quite confusing. But it looks good now!

Thanks @trevorrjohn!

@trevorrjohn trevorrjohn deleted the tj/snapshots-for-finders branch April 7, 2016 21:10
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.
2 participants
@trevorrjohn @pangratz