-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Context lifecycle in subscriptions #894
Comments
I had the same question in mind and piked at the code. It seems that we could add a I think it could be hooked here and should be a simple change. What do you think? |
@kouak Have you made any progress on this? Have you found a work-around? |
Sorry, no progress on this. My use case involves As far as I understand, your proposed implementation would not work. My understanding of the
If my understanding is correct, a |
maybe..., we can modify this: https://github.com/graphql/graphql-js/blob/master/src/subscription/subscribe.js#L149
|
FWIW, here's how i solved this:
Extra details:
|
I have a function called When I get a query or mutation over http then I call Socket connections for subscriptions are are more difficult. In the end I called In the I know this isn't super efficient but it's pretty fast, and could be turned into a lazy implementation if I need to. |
I just went to implement subscriptions for the first time and ran into this issue as well. Currently working around it using this patch using patch-package. What this allows us to do is specify a function const result = await graphqlSubscribe({
schema: graphqlSchema,
document,
variableValues: variables,
contextValue: makeDataContext('subscription'),
contextValueExecution(originalCtx) {
return makeDataContext('subscription-execution', { parent: originalCtx })
},
}) I think given the way folks currently use context as a sort of singleton "state container" in the query / mutation execution, a similar concept is necessary for subscriptions executions to work well without too many work-arounds. Particularly that is usually the reason you're re-triggering a subscription - because state has changed, you'd want to start from a fresh context. @IvanGoncharov let me know what you think of this approach, if it's something that would be reasonable to PR, and if so if the API naming is alright or should be changed. |
@tgriesser I really like this solution. The only problem I see, and please correct me if I'm wrong, but it seems this might be predicated on the subscription server being able to read context from the stateless (non-subscription) server? use case:
|
It shouldn't have anything to do with servers, this is purely an addition to allow the Any "graphql server", is really just a wrapper around the core |
gotcha, i think we just use different words. what you call a graphql server i call an execution server. there can also exist a subscription server, which does not execute graphql queries/mutations. In that case, graphql-js/src/subscription/subscribe.js Lines 194 to 197 in 4150d1f
|
to safely export buildExecutionContext() as validateExecutionArgs() motivation: this will allow us to: 1. export `executeOperation()` and `executeSubscription()` for those who would like to use directly. 2. add a `perEventExecutor` option to `ExecutionArgs` that allows one to pass a custom context for execution of each execution event, with the opportunity to clean up the context on return, a la #2485 and #3071, addressing #894, which would not require re-coercion of variables. The signature of the `perEventExecutor` option would be: ```ts type SubscriptionEventExecutor = ( validatedExecutionArgs: ValidatedExecutionArgs): PromiseOrValue<ExecutionResult> ``` rather than: ```ts type SubscriptionEventExecutor = ( executionArgs: ExecutionArgs): PromiseOrValue<ExecutionResult> ``` This might be a first step to integrating `subscribe()` completely into `execute()` (see: #3644) but is also a reasonable stopping place.
Closed by #4211 |
After reading through the codebase, my understanding of the subscription implementation is as follow :
subscribe()
when a subscription operation hits the server,subscribe()
returns anAsyncIterator
AsyncIterator
contains a stream of results of subsequent calls toexecute()
Again, if my understanding is correct,
contextValue
is passed tosubscribe
on the initial subscription operation and then passed down toexecute
whenever an event triggers a reexecution.In short,
contextValue
is set once a subscription operation happens and will remain the same until the client unsubscribes.A common graphql-js implementation is to use DataLoader as a batching/caching library. For queries and mutations, a set of fresh dataloader instances is usually bound to
contextValue
on a per request basis. When the query/mutation is done, those DataLoader instances are left for garbage collection.Now, subscription context has a much longer life span, making DataLoader caching behaviour unwarranted in most cases.
Ideally, I guess providing a way to set
contextValue
perexecute()
call would allow a behaviour that matches what one could except when settingcontextValue
on a per request basis for queries and mutations.Is there any way to do so in current implementation ? Is this a behaviour that could be implemented in graphql-js or should I settle for one context per subscription operation and work my way around that ?
The text was updated successfully, but these errors were encountered: