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

result transformer #378

Closed

Conversation

nevir
Copy link
Contributor

@nevir nevir commented Jul 11, 2016

Please note that this PR depends on, and includes #377. For a standalone diff, see convoyinc/apollo-client@store-context...store-result-transformer


This adds a resultTransformer callback that can be used to modify the results of a query just before they are handed off to the caller.

For example, we're crazy and like to attach a prototype to response objects, as a convenient place to hang read only helpers.

@nevir nevir force-pushed the store-result-transformer branch 2 times, most recently from 6aab46c to 0e593db Compare July 11, 2016 04:16
nevir added a commit to convoyinc/apollo-client that referenced this pull request Jul 11, 2016
Prompted by apollographql#376's brittleness; this helps keep common state/configuration around when performing a query.

---

Also, as part of this, I noticed there are two seemingly unused functions: `diffQueryAgainstStore`, `diffFragmentAgainstStore`
You can pass `resultTransformer` as an optional callbak to make changes to result objects before they're returned via the query interface.
nevir added a commit to convoyinc/apollo-client that referenced this pull request Jul 15, 2016
@nevir
Copy link
Contributor Author

nevir commented Jul 19, 2016

Is this a PR you guys would accept, or should I try a different approach?

@nevir nevir changed the title Store result transformer result transformer Jul 19, 2016
@stubailo
Copy link
Contributor

Hey sorry, I've been totally swamped with PRs, trying to get through them all as fast as possible. Started out with 17 this morning and down to 12 now...

@stubailo
Copy link
Contributor

stubailo commented Jul 19, 2016

I think this could be a good hook to have, so definitely in favor of it. But I'm not sure about the implementation. Why not just transform the whole result blob right before it's returned from watchQuery? Doesn't seem worth it to thread it all the way through to diffQueryAgainstStore etc.

@nevir
Copy link
Contributor Author

nevir commented Jul 19, 2016

Hey sorry, I've been totally swamped with PRs, trying to get through them all as fast as possible. Started out with 17 this morning and down to 12 now...

No rush, mostly just wanted to make sure it's worth maintaining & rebasing :)

I think this could be a good hook to have, so definitely in favor of it. But I'm not sure about the implementation. Why not just transform the whole result blob right before it's returned from watchQuery? Doesn't seem worth it to thread it all the way through to diffQueryAgainstStore etc.

Hmm, that I don't have a great answer for. I initially went down that road because I already had the context machinery (to support #376).

However, it does have the nice property that the callback fires for every returned node. If I got only a single call (right before watchQuery returns), I would have to turn around and walk the entire result to find those nodes again (for example, to look for anything with __typename).

But, the single callback approach allows for transformations that need to look at more than just a node at a time. I think your approach makes more sense, unless I've inadvertently convinced you otherwise

@stubailo
Copy link
Contributor

Yeah I just like the approach that has the least internal machinery. Just having a callback that runs on every result seems the most sense, and if we have a concrete reason to add something more complicated later we can do it then.

@stubailo
Copy link
Contributor

Gonna close this since the other approach will probably have totally different code!

@stubailo stubailo closed this Jul 19, 2016
@nevir
Copy link
Contributor Author

nevir commented Jul 19, 2016

Yup, SGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants