-
Notifications
You must be signed in to change notification settings - Fork 133
feat: hook into onExecuteSubscriptionEvent and useContextPerSubscriptionEvent #183
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
Conversation
🦋 Changeset detectedLatest commit: 3bb8394 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest changes of this PR are available as alpha in npm (based on the declared |
ca82cbf to
b8f1c2f
Compare
9d0fdf5 to
ab73859
Compare
ab73859 to
b667b3f
Compare
ce5a34b to
beba812
Compare
| const getEnveloped = envelop({ | ||
| plugins: [ | ||
| useContextValuePerExecuteSubscriptionEvent(() => ({ | ||
| contextValue: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@n1ru4l maybe we should append this context to the general one?
I think it might be an issue that we have separate context created here, and not having the original one as well, since resolvers might depend on the context.
| { | ||
| "name": "@envelop/execute-subscription-event", | ||
| "version": "0.1.0", | ||
| "author": "Dotan Simha <[email protected]>", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update this ;)
| * This is a almost identical port from graphql-js subscribe. | ||
| * The only difference is that a custom `execute` function can be injected for customizing the behavior. | ||
| */ | ||
| export const subscribe = (execute: ExecuteFunction): SubscribeFunction => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some plugins might use setExecuteFn during onExecute with the assumption that they override this function.
Does it mean that these plugins need somehow to override this function here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. just some minor changes :) (and we need a rebase :P )
This is the first attempt of trying to solve #80
Example Usage:
There are several things that are currently not optimal
1.
subscribeoverrideI created a copy of
subscribefromgraphql-js(which is surprisingly lightweight) and adjusted it so a customexecutefunction can be used for theExecuteSubscriptionEventphase.I assume that the reason why it is not possible to provide a custom
executeto the originalsubscribefromgraphql-jsis that the usage ofexecuteis considered an implementation detail.There is a pull request that attempts to allow overriding the
executepassed to subscribe but no update from thegraphql-jsmaintainers has been made recently. (graphql/graphql-js#2485) I also created graphql/graphql-js#3071The plugin hard overrides the
subscribefunction. I cannot imagine any use-cases right now where you would want to composesubscribefunctions (but I for example composeexecutefunctions within graphql-live-query, so there might be a niche for that also withsubscribe).Ideally, I would like to avoid having a copy of
subscribe, but I see no other option right now.2. Context composition hook compatibility
This does currently not work with context composition, as we use a custom factory for creating the context. It would be interesting to get more insight on this from different people. I usually use the same context factory function for all operation types (mutation, subscription, or query). So maybe instead of having a custom
createContextfactory function, we should instead construct the context the same way as forexecute(orsubscribe). Probably theuseContextPerSubscriptionValuecreateContextargument should be optional and instead, the implementation should use the default composition way of creating the context if the argument is not provided. Right now you would have to do sth like this: