-
Notifications
You must be signed in to change notification settings - Fork 131
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 <dotansimha@gmail.com>", |
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.
subscribe
overrideI created a copy of
subscribe
fromgraphql-js
(which is surprisingly lightweight) and adjusted it so a customexecute
function can be used for theExecuteSubscriptionEvent
phase.I assume that the reason why it is not possible to provide a custom
execute
to the originalsubscribe
fromgraphql-js
is that the usage ofexecute
is considered an implementation detail.There is a pull request that attempts to allow overriding the
execute
passed to subscribe but no update from thegraphql-js
maintainers has been made recently. (graphql/graphql-js#2485) I also created graphql/graphql-js#3071The plugin hard overrides the
subscribe
function. I cannot imagine any use-cases right now where you would want to composesubscribe
functions (but I for example composeexecute
functions 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
createContext
factory function, we should instead construct the context the same way as forexecute
(orsubscribe
). Probably theuseContextPerSubscriptionValue
createContext
argument 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: