Skip to content
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

Closed
wants to merge 40 commits into from

Conversation

DxCx
Copy link
Contributor

@DxCx DxCx commented Feb 17, 2017

TODO:

  • Update CHANGELOG.md with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

Closes #222
Closes #272

@stubailo
Copy link
Contributor

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.

@DxCx
Copy link
Contributor Author

DxCx commented Feb 23, 2017

Well @stubailo, i did a short overview in the cast we had, thats why i tought you got it.
Anyway, i do plan writing at least one blog post (or probably more) to help people adopt.

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,
The protocol should be supported by the existing subscription transport ws

@stubailo
Copy link
Contributor

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.

@DxCx
Copy link
Contributor Author

DxCx commented Feb 23, 2017

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..

@stubailo
Copy link
Contributor

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!

@DxCx
Copy link
Contributor Author

DxCx commented Feb 23, 2017 via email

@JustinElst
Copy link

Is there any way we can progress this pull request? From a quick look it looks good!

@DxCx
Copy link
Contributor Author

DxCx commented Mar 2, 2017

Thanks @MrGekko,
Atm we are in CR phase, waiting for @stubailo to give review notes.
hold on a bit i believe this will be merged soon then i'll start work on client side as well.

@DxCx DxCx force-pushed the graphql-server-ws-core branch 2 times, most recently from 8592048 to c1507f7 Compare March 22, 2017 10:55
This was referenced Mar 22, 2017
@DxCx DxCx force-pushed the graphql-server-ws-core branch 2 times, most recently from 3dc8613 to 25723a7 Compare March 29, 2017 07:36
DxCx added 25 commits April 9, 2017 09:54
@DxCx
Copy link
Contributor Author

DxCx commented May 18, 2017

Replaced by the work done on apollographql/subscriptions-transport-ws#108

@DxCx DxCx closed this May 18, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants