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

Allow include query parameter with store.findRecord & store.findAll #3976

Merged
merged 1 commit into from
Dec 16, 2015
Merged

Allow include query parameter with store.findRecord & store.findAll #3976

merged 1 commit into from
Dec 16, 2015

Conversation

HeroicEric
Copy link
Member

Related RFC: emberjs/rfcs#99

Description

These changes allow an include parameter to be specified when using
store.findRecord and store.findAll:

store.findRecord('post', 123, { include: 'comments' });
store.findAll('post', { include: 'comments' });

The value for include is copied onto the DS.Snapshot or
DS.SnapshotRecordArray that is passed to the corresponding adapter
function. This value is then pulled from the snapshot and used as a
query parameter (include) when making an AJAX request via
adapter.ajax.

Questions

  1. Is it OK that the include option is copied into adapterOptions or should it be copied into adapterOptions.query. I originally copied it into adapterOptions.query but the code was a little uglier so I went with just copying it into adapterOptions, but I'm still not sure.
  2. If the value for include is an array, should we automatically join the values into a comma separated list? The JSON API spec for "Inclusion of Related Resources" requires that the value be a comma separated list but I didn't think that was necessarily what everyone would want. We could just do that in the JSON API adapter but I figured I'd ask for some feedback before proceeding with that.

Work to be done in additional PRs

It was also mentioned in the RFC that we could provide some additional functionality:

post.save({ include: 'comments' });
post.destroyRecord({ include: 'comments' });

I'm not totally clear on what the behavior would be there yet so those were left unimplemented for now.

@bmac
Copy link
Member

bmac commented Dec 8, 2015

  1. Is it OK that the include option is copied into adapterOptions or should it be copied into adapterOptions.query. I originally copied it into adapterOptions.query but the code was a little uglier so I went with just copying it into adapterOptions, but I'm still not sure.

I think copying into adapterOptions is the best thing to do here.

  1. If the value for include is an array, should we automatically join the values into a comma separated list? The JSON API spec for "Inclusion of Related Resources" requires that the value be a comma separated list but I didn't think that was necessarily what everyone would want. We could just do that in the JSON API adapter but I figured I'd ask for some feedback before proceeding with that.

I was originally against this because I think it would be more consistent if the API only accepted 1 type. However, @igorT convinced me that allowing the value to be an array could allow Ember Data to be smarter and warn if an included relationship was not defined on the model.

@wecc
Copy link
Contributor

wecc commented Dec 8, 2015

I agree with @bmac

bmac
bmac reviewed Dec 8, 2015
View reviewed changes
addon/adapters/rest-adapter.js
const adapterOptions = snapshotRecordArray.adapterOptions || {};
const { include } = adapterOptions;

let query;
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind refactoring some of the duplicated code in findRecord and findAll into a helper method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

@bmac
Copy link
Member

bmac commented Dec 11, 2015

@HeroicEric Do you mind wrapping all your changes in feature flags?

@HeroicEric HeroicEric mentioned this pull request Dec 12, 2015
Merged
@HeroicEric HeroicEric changed the title Allow include query parameter with store.findRecord & store.findAll WIP - Allow include query parameter with store.findRecord & store.findAll Dec 13, 2015
@HeroicEric HeroicEric changed the title WIP - Allow include query parameter with store.findRecord & store.findAll Allow include query parameter with store.findRecord & store.findAll Dec 13, 2015
@HeroicEric
Copy link
Member Author

@bmac I tried wrapping the changes in a feature flag but I'm not sure I did all the steps correctly. The feature flagged tests don't seem to run with or without the --environment=test-optional-features flag or with the "Enable Opt Features" boxed checked.

Edit: Nevermind. Figured it out. Features aren't meant to be inside a "features" key in the features.,json

HeroicEric
HeroicEric reviewed Dec 13, 2015
View reviewed changes
tests/integration/adapter/rest-adapter-test.js
@@ -56,7 +57,7 @@ test("find - basic payload", function(assert) {
run(store, 'find', 'post', 1).then(assert.wait(function(post) {
assert.equal(passedUrl, "/posts/1");
assert.equal(passedVerb, "GET");
assert.equal(passedHash, undefined);
assert.deepEqual(passedHash.data, {});
Copy link
Member Author

Choose a reason for hiding this comment

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

Would it be preferred to continue returning undefined rather than {} in these cases? adapter.ajax seems to act the same with { data: {} } and {}.

Copy link
Member

Choose a reason for hiding this comment

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

returning {} seems ok to me.

@bmac bmac removed the team-review label Dec 13, 2015
@HeroicEric
Allow include query parameter with store.findRecord & store.findAll
266d3b0 
Related RFC: emberjs/rfcs#99

These changes allow an `include` paremeter to be specified when using
`store.findRecord` and `store.findAll`:

```javascript
store.findRecord('post', 123, { include: 'comments' });
store.findAll('post', { include: 'comments' });
```

The value for `include` is copied onto the `DS.Snapshot` or
`DS.SnapshotRecordArray` that is passed to the corresponding adapter
function. This value is then pulled from the snapshot and used as a
query parameter (`include`) when making an AJAX request via
`adapter.ajax`.
@HeroicEric
Copy link
Member Author

Rebased, updated the description, added the feature flag to FEATURES.md. Anything else I can do here? Other than start writing some docs 😄

bmac added a commit that referenced this pull request Dec 16, 2015
@bmac
Merge pull request #3976 from HeroicEric/allow-include
e1c993e 
Allow `include` query parameter with store.findRecord & store.findAll
@bmac bmac merged commit e1c993e into emberjs:master Dec 16, 2015
@bmac
Copy link
Member

bmac commented Dec 16, 2015

🎉

@HeroicEric HeroicEric deleted the allow-include branch December 16, 2015 17:17
@bmac bmac mentioned this pull request Feb 13, 2016
Closed
@pangratz pangratz mentioned this pull request May 16, 2016
Closed
@HeroicEric HeroicEric mentioned this pull request Sep 20, 2016
Closed
MarkMT added a commit to MarkMT/data that referenced this pull request Oct 17, 2016
@MarkMT
[DOC] Update descriptions of findRecord() and findAll()
b52399b 
This commit adds documentation on the use of the `include` query parameter in `DS.Store` methods `findRecord()` and `findAll()`. This feature allows records of models related to those requested to be retrieved from JSON API compliant server in a single request. This capability was added in emberjs#3976 but is not currently documented.
@MarkMT MarkMT mentioned this pull request Oct 17, 2016
Merged
runspired pushed a commit to runspired/data that referenced this pull request Oct 19, 2016
@MarkMT @runspired
[DOC] Update descriptions of findRecord() and findAll()
6c72f14 
This commit adds documentation on the use of the `include` query parameter in `DS.Store` methods `findRecord()` and `findAll()`. This feature allows records of models related to those requested to be retrieved from JSON API compliant server in a single request. This capability was added in emberjs#3976 but is not currently documented.
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.
3 participants
@HeroicEric @bmac @wecc