introduce executeSubscriptionEvent - #3657
Conversation
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
The `execute`/`executeImpl` and `createSourceEventStream`/`createSourceEventStreamImpl` functions follow the same basic pattern of building the contet and using it to run a function. This PR extracts that pattern into a separate function. For good measure, the same pattern in applied to the soon-to-be-deprecated `subscribe` function. Hheavier refactoring is on the way from @IvanGoncharov (see graphql#3639 (review)), but in the meantime, this consolidates the common pattern without any breaking changes.
...to executeSubscriptionRootField => because that's what it does!
...because that's what it does! The execution of an operation returns a map of data/errors, not just the data.
ab12f8a to
dce4720
Compare
`execute` no longer runs the query algorithm for subscription operations. Rather, subscription operations are performed, as per the spec. `subscribe` is deprecated.
to replace the old exported functionality from execute
dce4720 to
c23fd59
Compare
yaacovCR
added
PR: feature 🚀
IvanGoncharov
left a comment
IvanGoncharov
left a comment
There was a problem hiding this comment.
So we are back to square one where we duplicate work done by createSourceEventStream.
Adding a new API means we can't easily change or remove it so we need to solve the problem of duplicating work.
In #3658 I proposed exact same API but without this problem.
Merging this PR restricts design space for figuring out how to remove this duplication.
So I want to resolve the problem of duplication before merging it, so we sure API is compatible with this solution.
I think it's fair to block this change since a full alternative is provided in #3658
|
I don't understand, this is for use in a separate service that is set up to only run this function to manage subscriptions, so there is duplicative work of coercing because it is done by a separate service. We have discussed already how CreateSourceEventStream is not commonly used but this is available now for those who need it. I do not see how your PR provides an alternative without this additional work if indeed run as two services. |
2 participants
to replace the old exported functionality removed from execute by #3644
depends on #3644