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

Websocket support (WIP) #1443

Closed
wants to merge 12 commits into from
Closed

Websocket support (WIP) #1443

wants to merge 12 commits into from

Conversation

DxCx
Copy link
Contributor

@DxCx DxCx commented Mar 18, 2017

Notes:

  1. This PR adds support for websocket transport, pending support on server: GraphQL-server websocket support apollo-server#308
  2. It's not ready yet, but i've opened the PR so you can start review and see changes.
  3. This implementation uses browser native WebSocket client, therefore no extra dependancies are needed (Only websocket polyfill for node for testing)
  4. because we eventually want to create a repo for all network interfaces, i think it can just be apart of apollo client until then..
  5. query is just a wrap of _query which supports Observable response. so when Observable query will be supported, that's a quick fix.
  6. (@helfer) as we discussed, middlewares are not supported atm, as websocket doesn't initiate a new request for each packet. let's think how and if we want to support them.

TODO:

  • Make Online instance of SWAPI GraphQL-WebSocket.
  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Need to add documentation - @helfer which needs update?
  • Rebase your changes on master so that they can be merged easily
  • Update CHANGELOG.md with your change
  • General Polish + Unit testing
  • Add HTTP+WS Hybrid mode.
  • Analyze Binary size and reduce it:
    • BUG: websocket library is being bundled
    • url lib is too big, better to split string manually then pay for the size.
  • Pass options as 'init' message.
  • Add your name and email to the AUTHORS file (optional)
  • Implement subscriptions.
  • Make sure all of the significant new logic is covered by tests
  • Make sure all tests and linter rules pass

@helfer
Copy link
Contributor

helfer commented Mar 22, 2017

Thanks for the PR @DxCx ! As discussed in person, let's break this into a separate package and a small PR that just adds observable support to the current Network interface API, and makes sure the API is aligned with the fetcher used in graphiql. Basically, the PR to Apollo Client should only make the following changes:

  1. Make sure all places where Apollo Client calls the Network Interface can accept either a promise or an observable as a return value.
  2. Make sure all places where Apollo calls the network interface can deal with two kinds of network interface: 1) the old one, which has a query method on it that is used for queries and mutations, and 2) the new one, which has only an execute function that takes arguments in the same shape as the GraphiQL fetcher ( https://github.com/graphql/graphiql ).

For the rest of this PR, one part of it should go into the subscriptions transport, which we'll rename to graphql-transport-ws once it supports all operations ( if @Urigo is okay with that).

The other part should go into a separate WS network interface package, which has the new execute function on it and always returns an observable.

I'm really excited about these changes, and I think we can put together a really cool demo once we have all the pieces in place! 😀

@helfer helfer closed this Mar 22, 2017
@smolinari
Copy link
Contributor

Has the graphql-transport-ws change been moving forward somewhere else as suggested? I'd like to keep track of any updates. I too am excited for these changes. 👍

Thanks.

Scott

@DxCx
Copy link
Contributor Author

DxCx commented Apr 11, 2017 via email

@smolinari
Copy link
Contributor

Super news! Thanks for the update. Any rough ETA? No worries, I won't hold you to it. I just need a rough idea of the timeline.

Scott

@DxCx
Copy link
Contributor Author

DxCx commented Apr 11, 2017

actually i didn't have much time to do the PR myself,
but thanks to @mistic client side is pretty much ready, we are working on server.
and then we will need to refine the testing abit.
i believe in the next week we will open a PR in graphql-transport-ws

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

Successfully merging this pull request may close these issues.

None yet

3 participants