-
Notifications
You must be signed in to change notification settings - Fork 103
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
✨ Added graphql-ws #301
✨ Added graphql-ws #301
Conversation
Ping @aexol, there are a few merge conflicts now, are you working on this? Can I be of any help? |
Hello @aexol, could you update us on the status of this please ? There are no reviews, no feedbacks, and it is hard for us to know if we can expect this feature that we really really really need merged at some point or if we have to fork the project on our side. |
Sorry I thought it is not completed. Why not make a plugin instead of forcing graphql-ws to the main code? |
It looks like Zeus support subscriptions out of the box but that's not the case because the implementation of apiSubscription is non standard. This PR is more of a fix to add the standard implementation to the core, as a non-default feature flag to keep the current behavior, rather a new feature. This PR will make Zeus compatible with Apollo and Yoga subscriptions that both follow this standard. I haven't written the documentation yet because I wanted to make sure that you were okay with changes to the core. If you agree on this design, I'll complete the todo list. Also, I chose to implement it as a non-breaking change; both the current implementation and graphql-ws currently coexist in generated.ts: https://github.com/graphql-editor/graphql-zeus/pull/301/files#diff-96a2264dfde03a31fbad8f2aac04c691dd6a7291919fbcbf258ba30782f394daR664 Then, you may or may not make graphql-ws the default in the next major version. |
If it won't break I am ok go ahead and finish it. It looks like you are implementing ts with strings. You don't have to do it anymore. Just use pure ts files and produce-lib script |
Great news! I'll try to have it ready by the end of the week.
Not really, I did it the same way as you:
|
58ec55d
to
68cc80a
Compare
Ready for review! I feel like a lot of things are wrong: The type
|
Thanks @GauBen I'll try to review next days |
Thank you! I'll sort the merge conflicts out soon. |
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.
Looks nice! Please fix those small errors and suggestions, so we can merge and release next version 🚀
Thanks for the review! You'll find all the changes in 6be6e47 . |
🎉 |
Please test |
Thank you! |
Everything works on my side! 🎉 |
Working to fix #288
open
andoff
, if they still make sense