Add a new GraphiQL fetcher #96
Conversation
src/graphiql-fetcher.ts
Outdated
| subscriptionsClient.unsubscribe(activeSubscriptionId); | ||
| } | ||
|
|
||
| if (subscriptionsClient && graphQLParams.query.startsWith('subscription')) { |
There was a problem hiding this comment.
This seem like a brittle way to test if it is a subscription.
There was a problem hiding this comment.
@NeoPhi Agreed.
I thought to try to do it without adding a dependency to grpahql.js...
(GrpahiQL is dependent on it but doesn't expose it)
But can definitely change that. what do you think would be better?
There was a problem hiding this comment.
I agree with @NeoPhi. Instead think you can do pretty much what this does:
If you don't want to add the graphql-js dependency, you can write that in the comment there and just use any for all the types. But you should definitely respect the operationName as well. GraphiQL is one of the few cases where it's actually useful for people to have many definitions in a document but only execute one. This GraphQL fetcher should support that.
There was a problem hiding this comment.
GraphiQL exposes string to the fetcher, and not GraphQL parsed object (DocumentNode). So we have two options:
- Add dependency for
graphql, parse the string from GraphiQL and use it to check for subscriptions operations. - Leave as is - maybe look for a substring instead only
startsWith, or maybe other logic you think is better.
@NeoPhi @helfer what do you think is the preferred solution? or maybe I'm missing a third one?
There was a problem hiding this comment.
GraphiQL already comes with graphql. Is there a way to get it from that bundle? If not, I would suggest this:
- if
operationNameis set, look for a match forsubscription <operationName>. - if
operationNameis not set, check if subscription is present somewhere before the first{in the string.
I think we should only send it over the subscription transport if we're very confident that a subscription should happen, because the error messages from subscriptions aren't as easy to understand as the ones that are returned from the normal graphql executor.
There was a problem hiding this comment.
As this is for GraphiQL I don't think the added weight of pulling in GraphQL is too much of a concern.
src/graphiql-fetcher.ts
Outdated
| let activeSubscriptionId: number | null = null; | ||
|
|
||
| return (graphQLParams: any) => { | ||
| if (subscriptionsClient && activeSubscriptionId !== null) { |
There was a problem hiding this comment.
While it would require GraphiQL changes to support stopping a subscription, having multiple active subscriptions would be ideal.
There was a problem hiding this comment.
mmmm because this is a temporary change until this PR would be merged, it's hard for me to think how keeping multiple subscriptions would look nice in the current GraphiQL UI.
Think that you would have a new subscription but would still keep getting new results of the previous subscriptions. probably would be confusing.
But maybe it would be an interesting thing to think about a better UI for a deeper change in GraphiQL itself.
Do you still think we should leave the previous subscriptions active or leave it like it is now?
There was a problem hiding this comment.
This looks great, but we should make sure we have a solid way of checking if the operation under operationName is a subscription. See the link to get an idea for how to do it (although, use the operationName) instead of a for loop, if it exists.
Some tests for this might be good as well, to make sure we support the main use-cases.
src/graphiql-fetcher.ts
Outdated
| subscriptionsClient.unsubscribe(activeSubscriptionId); | ||
| } | ||
|
|
||
| if (subscriptionsClient && graphQLParams.query.startsWith('subscription')) { |
There was a problem hiding this comment.
I agree with @NeoPhi. Instead think you can do pretty much what this does:
If you don't want to add the graphql-js dependency, you can write that in the comment there and just use any for all the types. But you should definitely respect the operationName as well. GraphiQL is one of the few cases where it's actually useful for people to have many definitions in a document but only execute one. This GraphQL fetcher should support that.
Urigo
force-pushed
the
feat/graphiql-fetcher
branch
from
8d12840 to
0e97395
Compare
|
@helfer @NeoPhi |
Urigo
force-pushed
the
feat/graphiql-fetcher
branch
from
5fdf14a to
769220a
Compare
Urigo
force-pushed
the
feat/graphiql-fetcher
branch
from
769220a to
115e4fa
Compare
# Conflicts: # CHANGELOG.md
|
Looks good to me! |
use single quotes on import filename
Add a new GraphiQL fetcher that overrides the default one so we can easily add GraphQL subscriptions support to GraphiQL