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

Include param defined in the adapter #10

Open
bmac opened this issue Feb 23, 2015 · 6 comments
Open

Include param defined in the adapter #10

bmac opened this issue Feb 23, 2015 · 6 comments

Comments

@bmac
Copy link
Member

bmac commented Feb 23, 2015

@bobholt @tkellen @cowboy @leobalter What is your reaction to the idea of defining the include parameter on the adapter? I was thinking it would end up looking something like this.

// app/adapters/post.js

import EndpointsAdapter from "ember-data-endpoints/adapter";
export default EndpointsAdapter.extend({
  queryParams: {
    include: ['author', 'comments', 'comments.author'].join(',')
  }
});

Then any time you would need to do a get GET request the adapter's queryParams object would be serialized into the url. For example store.find('post') would request GET /posts?include=author,comments,comments.author and store.find('post', 1) would request GET /posts/1?include=author,comments,comments.author.

I think in the cases where doing a store.find('post', {include: 'foo'}) the include: 'foo' would take priority over the the include defined on the queryParams object.

Open question. Should the queryParams be included in the url for non get requests?

@tkellen
Copy link

tkellen commented Feb 23, 2015

So, would you need to define a custom adapter for each model then?

@bmac
Copy link
Member Author

bmac commented Feb 23, 2015

Yes, for each model that has relationships.

@leobalter
Copy link
Contributor

👍 on defining the include parameter on the adapter.It might be good to have it avoiding duplicity on different routes.

@cowboy
Copy link

cowboy commented Feb 23, 2015

Part of me likes defining the include in the adapter as part of the low-level queryParams. The other part of me wants it defined in the route, where you'd be specifying options to store.find. Then again, another part wants it defined in the model, where you'd be setting async. But maybe async really shouldn't be in the model. I dunno.

Either way, the join(',') part seems really ugly, although I know why it's technically necessary.

Where is the most Embery place to define sideloaded relationships?

@bmac
Copy link
Member Author

bmac commented Feb 23, 2015

The join(',') part was just me being cute it could have been written more simply as a string. I feel like making this an adapter concern is correct because it involves the url which Ember Data is using to communicate with the backend. The problem with store.find params is because they are opaque to Ember Data it becomes hard for Ember Data to know if it can cache the resulting record. Also, and I'd love to tease out if I'm wrong with this theory, but I suspect that the includes params are relatively static for a type throughout an application.

As for async I think you 100% correct and the async property doesn't really belong in the model definition.

@tkellen
Copy link

tkellen commented Feb 24, 2015

I think you're probably right that these are mostly static throughout the app. Let's give it a shot on our next internal app using this (away) and see how it goes?

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

4 participants