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

Allow store.find to accept query-params with id (Closes #1576). #1579

Closed
wants to merge 2 commits into from

Conversation

abuiles
Copy link
Member

@abuiles abuiles commented Dec 16, 2013

This is a first stab at supporting extra query params when requesting an specific id.

@abuiles
Copy link
Member Author

abuiles commented Dec 16, 2013

I'm thinking about how to handle model.reload(), shall we "remember" the params used for finding a record, or just allow reload to accept a query params. I'm more inclined towards accepting queryParams in the reload, it might not be responsibility of the record to know that.

@bradleypriest
Copy link
Member

This feels like quite an edge-case to me. I'd like to see a better use case than {include: "comments"} before we add this. Maybe put it up on the discussion forum first.
Cheers

@abuiles
Copy link
Member Author

abuiles commented Dec 16, 2013

@bradleypriest This is related with #1576 please check the comments there, this has been adopted
recently in jsonapi, which I think we are taking as reference. Actually @wycats was the one accepting the change in jsonapi
json-api/json-api#145

@abuiles
Copy link
Member Author

abuiles commented Dec 16, 2013

This could also help resolve #51

@abuiles
Copy link
Member Author

abuiles commented Dec 16, 2013

@abuiles
Copy link
Member Author

abuiles commented Dec 17, 2013

@wycats could you please have a look at this? Would make more sense to have this just in the JSONAPI adapter?

@abuiles
Copy link
Member Author

abuiles commented Jan 3, 2014

@igorT ping

@ghost ghost assigned wycats Jan 3, 2014
@igorT
Copy link
Member

igorT commented Jan 7, 2014

I think the reload case is a bit tricky. In the json api spec, query params are used for requesting sideloading. In my mind a reload is tied to a specific record that you are reloading. If you are doing a reload and you pass in {include:comments} should the comments also go into a reloading state?

@igorT
Copy link
Member

igorT commented Jan 7, 2014

@wycats thoughts?

@wycats
Copy link
Member

wycats commented Jan 14, 2014

As @igorT points out, this doesn't work for all cases. I'd like to think about this more; at present, it feels too much like a scenario-solve than a comprehensive solution.

@stefanpenner
Copy link
Member

we should likely enumerate the scenarios this must work for, this will likely guide the correct solution.

@adamlc
Copy link

adamlc commented Feb 10, 2014

Any updates on this? :)

@kodayashi
Copy link

Hi @abuiles, was this merged? The third parameter seems to be ignored in the latest release.

@abuiles abuiles deleted the find-with-id-and-query-params branch October 14, 2014 18:15
@abuiles
Copy link
Member Author

abuiles commented Oct 14, 2014

@kodayashi no, it wasn't merged.

@kodayashi
Copy link

Gotcha, thanks for the update @abuiles. I believe @stefanpenner is looking for use cases here. Currently, I'm looking to use query parameters for a given 'model' object opposed to an entire collection of model objects:

(theoretical) -- store.find('model', 1, {filter: 'filter_value'}) -- Only one model object needs to be loaded. Query param is then applied against object.

(today) -- store.find('model', {id: 1, filter: 'filter_value'}) -- Entire model collection is loaded, then one model is filtered, then query param applied.

For our use case, the param defines a filter for a Graph object that's associated with the 'model' object. Hope that helps.

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

Successfully merging this pull request may close these issues.

None yet

7 participants