-
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
GraphQL-server websocket support #308
Conversation
1db893d
to
4d14c95
Compare
I think this could be really cool! Could you maybe write up a bit of a design about what this is doing and how it changes some of the core packages? I saw some of the discussion on #272 but I think it would be great if there was a concise description here of what the transport looks like, how this affects people using the HTTP implementation, etc. |
Well @stubailo, i did a short overview in the cast we had, thats why i tought you got it. The base protocol is also documented mostly on the issue atm but as you can see i am inteding to format it abit more information and at least add .md under protocol package (adding some basic explanation on apollo docs site, pointing to this md, for those who wants to dig deeper - can be pretty neat) Btw, |
Yeah my thought was more about the implementation design - I understand the concepts of what it's trying to achieve. Basically it's hard to review the code if there isn't a description of what it's trying to do on an implementation level. For example, I see that we are including an observable polyfill rather than using RxJS - would it be better to use RxJS instead? It would be interesting to hear about some of the design decisions. |
Well, IMO it is worth to use rxjs instead of observable polyfill, it will make code much more readable, however, ive been requested by helfer to support observable the exact same way apollo client does which is with polyfills. So my hope is that in the future observables will become part of es7 and we can eventually remove all polyfills at least. Anyway, the ticket was open for discussion for around 2 weeks before i started the implementation, if there are gaps there please write in the issue all the gaps so i can feel them for you and hopefully for everyone else in the future.. |
Yes I agree completely with everything presented in the issue - the only thing I'm saying is that I'd like to review this but it's over 2000 lines of code. So it would be good to have some help about where to start and what to look at. The issue mostly goes over how it will be used and the protocol, which is great! |
Alright,
But the cool thing is that its splitted into packages
So i would not start with a diff but rather with the final branch,
reviewing the new packages, and then look at the small modifications of the
existing files
IMO, this should be the order:
1. Observable
2. Reactive-core
3. Reactive-protocol
4. Websocket
…On Thu, 23 Feb 2017 at 22:56 Sashko Stubailo ***@***.***> wrote:
Yes I agree completely with everything presented in the issue - the only
thing I'm saying is that I'd like to review this but it's over 2000 lines
of code. So it would be good to have some help about where to start and
what to look at. The issue mostly goes over how it will be used and the
protocol, which is great!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#308 (comment)>,
or mute the thread
</~https://github.com/notifications/unsubscribe-auth/ABB8WyERbOp4gjiRcNyHlGeaqveWJPVmks5rfgCCgaJpZM4MEcov>
.
|
Is there any way we can progress this pull request? From a quick look it looks good! |
8592048
to
c1507f7
Compare
3dc8613
to
25723a7
Compare
See apollographql#272 for more info.
25723a7
to
f9466be
Compare
Replaced by the work done on apollographql/subscriptions-transport-ws#108 |
TODO:
Closes #222
Closes #272