(feat) Add support for the top-level extensions key in the GraphQL specification #355

adamscybot · 2019-07-26T14:09:49Z

Resolves #353.

…tion

JoviDeCroock

Looks very good to me, thanks for taking your time to contribute this! 👍

src/hooks/useMutation.test.tsx

docs/architecture.md

kitten · 2019-07-26T14:21:10Z

src/hooks/useSubscription.ts

        setState(s => ({
          fetching: true,
          data: handler !== undefined ? handler(s.data, data) : data,
          error,
+          extensions,


I don't think we should pass this on to the "user-level" as exchanges would be the only consumers of these special fields.

Or is there any use case you can think of? I'm not sure whether Apollo passes them on?

This can especially confusing because cached results wouldn't have any extensions, which makes this an easy pitfall as well

I don't think its the case that exchanges are the only consumers -- what's in there is entirely arbitrary and actually could have functional reasons. For us, we are passing down informations regarding authorisations and cross-cutting UI concerns that arent actually in the "graph". This is quite a purist view of graphQL -- but there are definitely use cases.

Another use case might be to surface some performance metrics in the UI.

This can especially confusing because cached results wouldn't have any extensions, which makes this an easy pitfall as well

Ah, I was hoping they would come in. I think my changes to the cacheExchange sees to that?

Heres an example we actually have in production: Sometimes a user can see part of an entity but they aren't allowed to see the wider detail of it -- hence, they cant "click through" on these entities. Data about this is sent in the extensions and the UI reacts to it. We dont send this data in the data itself as its very much meta and a cross cutting thing that can affect any entity.

Apollo doesn't pass them on. But it's because its a bug: apollographql/react-apollo#2059. The intended fix in apollo is to cache these things alongside everything else.

I think this very much could also be part of the data bit fair enough. The problem here though is that we have to assume that the extensions are bound to the specific request.

This means that we shouldn't cache it as it's stale when it's read back from the cache, especially if it contains tracing data and performance data. This probably should also not be included in the SSR result as the frontend shouldn't be concerned about request information from the backend.

I know this is controversial, but I have the feeling that this is very much "non best-practice territory".

If we're thinking of authorisation in general, if it's meta data then a custom exchange would possibly handle that. Otherwise it may be better to include it in data.

A normalised cache would never be able to cache this data, as queries can be fulfilled entirely offline.

So there's definitely a point I believe to surfacing this to the user for "advanced use cases" but the extensions probably shouldn't be cached, shouldn't be part of the SSR serialised data, and shouldn't be guaranteed.

I'm unsure however about some details around this 😅

Sure, I can see where you are coming from. It doesn't help the spec for extensions is essentially "object for stuff". We ourselves have wondered hard if we are overloading this thing. We could stretch the definition of an error for some of these use cases and use that. Those have a path in the spec back to the data and so are inherently linked.

If the extensions relate to the data, they could be cached. If the extensions don't relate directly to the data (perf metrics), they should not be cached. You're right, thats "custom exhcnage" territory.

Back to the drawing board :(. I guess my next questions is should I just limit this whole PR to the fetchExchange? People could then do what they want with them in their own exchanges and potentially opt to put them in the data where they will be cached.

It works for our use case and isn't making any assumptions about what is in there for everybody else.

I mean, I'm definitely all for passing extensions on. Yea, that's a nice way of getting some extra data out of exchanges.

But none of the default behaviour should cache or guarantee the extensions. That's definitely custom exchange territory.

So the ssrExchange should probably be modified to remove extensions from the serialised result.

Right, so just so Im 100% clear before I sort this out: We want to surface extensions in the hooks, we just want to remove my bad assumption re: caching the extensions?

I think as well as ssrExchange i need to modify cache.ts to filter extensions before it does a cache set operation.

We want to surface extensions in the hooks, we just want to remove my bad assumption re: caching the extensions?

yea, let's surface it. We probably won't put too much focus on it in the docs, but it's particularly useful for also adding (client-only) information/metadata/data from the exchanges, without modifying data, like you said.

I think as well as ssrExchange i need to modify cache.ts to filter extensions before it does a cache set operation.

I think you might need to keep it when you set, because otherwise it may be missing during SSR after suspense is done 🤔 But we definitely want to filter it out from the data that is returned by serializeData, so that it's not sent to the client from the server.

Cheers for taking the time to do this btw 🙌

I've committed a fix to ssrExchange to not add extensions during serialisation.

I think I'm getting slightly confused over the cache in ssrExchange and cacheExchange. In cacheExchange I've now removed the setting of extensions in there by filtering it -- I think we might accidentally be referring to different caches in above conversations.

Might be best for you to sanity check what I've just done.

Yea, I'll give it a quick review again 👌

…ponse

src/exchanges/cache.ts

kitten · 2019-07-26T16:08:15Z

so, on exchanges/ssr we need to filter out the extensions on ssr.extractData, since that's the ssr cache data that will be serialized and sent to the client. Unfortunately it won't be enough to remove it when we deserialize 😅

adamscybot · 2019-07-26T16:17:21Z

so, on exchanges/ssr we need to filter out the extensions on ssr.extractData, since that's the ssr cache data that will be serialized and sent to the client. Unfortunately it won't be enough to remove it when we deserialize 😅

I think we are fine because serializeResult already only considers data and errors and not extensions so it does not get serialised (I think anyway...the SSR part I'm the least certain about).

kitten · 2019-07-29T11:12:28Z

Yep, that's right! I've reviewed the full thing now and it looks perfect! I'll merge it and see if we need more tests and start seeing whether it all behaves as expected 👍

Thanks again for taking the time to submit this!

adamscybot · 2019-07-29T12:01:54Z

Thanks! Happy to help.

Adam Thomas and others added 3 commits July 26, 2019 14:38

Add support for the top-level extensions key in the GraphQL specifica…

b8fe810

…tion

Add support for the top-level extensions key in the GraphQL specifica…

483a41b

…tion

Merge branch 'master' of https://github.com/adamscybot/urql

ced53da

JoviDeCroock approved these changes Jul 26, 2019

View reviewed changes

src/hooks/useMutation.test.tsx Outdated Show resolved Hide resolved

kitten reviewed Jul 26, 2019

View reviewed changes

adamscybot added 3 commits July 26, 2019 15:22

Make hook fixture for extensions more respresentational of a real res…

1d389f9

…ponse

Fix ambiguity in docs about extensions: they aren't typical

d1e8e2d

Don't cache extensions, its an unsafe assumption

69fc52a

kitten reviewed Jul 26, 2019

View reviewed changes

src/exchanges/cache.ts Show resolved Hide resolved

kitten approved these changes Jul 29, 2019

View reviewed changes

kitten merged commit 09494f2 into urql-graphql:master Jul 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(feat) Add support for the top-level extensions key in the GraphQL specification #355

(feat) Add support for the top-level extensions key in the GraphQL specification #355

adamscybot commented Jul 26, 2019

JoviDeCroock left a comment

kitten Jul 26, 2019

adamscybot Jul 26, 2019

adamscybot Jul 26, 2019 •

edited

Loading

kitten Jul 26, 2019

adamscybot Jul 26, 2019 •

edited

Loading

kitten Jul 26, 2019

adamscybot Jul 26, 2019

kitten Jul 26, 2019

adamscybot Jul 26, 2019

kitten Jul 26, 2019

kitten commented Jul 26, 2019

adamscybot commented Jul 26, 2019

kitten commented Jul 29, 2019

adamscybot commented Jul 29, 2019

(feat) Add support for the top-level extensions key in the GraphQL specification #355

(feat) Add support for the top-level extensions key in the GraphQL specification #355

Conversation

adamscybot commented Jul 26, 2019

JoviDeCroock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamscybot Jul 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamscybot Jul 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitten commented Jul 26, 2019

adamscybot commented Jul 26, 2019

kitten commented Jul 29, 2019

adamscybot commented Jul 29, 2019

adamscybot Jul 26, 2019 •

edited

Loading

adamscybot Jul 26, 2019 •

edited

Loading