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

Reactive GraphQL #809

Closed
wants to merge 13 commits into from
Closed

Reactive GraphQL #809

wants to merge 13 commits into from

Conversation

xareelee
Copy link

@xareelee xareelee commented Apr 16, 2017

This PR is only for discussion about Reactive GraphQL we discussed before in the #647 (comment).

Here are what I did in January (sorry for delayed PR):

  • Rewrite the graphql.js using RxJS to replace Promise-based graphql.js (all tests passed, you could run ). Run yarn run testonly-rx.
  • Rewrite the graphql.js using most.js to replace Promise-based graphql.js (one test failed, still unresolved). Run yarn run testonly-most.
  • Write benchmark.js to test the performance for Promise-based, RxJS-based and Most.js-based graphql(). Run yarn run benchmark.

Current progress:

  • Finish a refactored graphql version for reactive programming.
  • Pass tests (RxJS-based version)

Current Issues:

TODO (if we agree to keep going this fork):

  • Figure out the memory issues using RxJS and most.js with GraphQL.
  • Make sure it is useful for real world and add more tests for reactive programming.
  • Further refactoring to remove using exeContext.errors.push and use reactive Observable.error() instead of throw for error-handling.
  • Rewrite the graphql.js with callback functions or our own version of Monad for Reactive Programming without any 3rd-party Rx dependency.
  • Fix code for flowtype and lint.
  • Make performance better (at least not significantly worst than Promise-based graphql).

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact [email protected] if you have any questions.

@Birowsky
Copy link

Birowsky commented Aug 2, 2017

@xareelee i would love to know whether there is any progress here or whether you figured out what caused the memory issue with rx.

@xareelee
Copy link
Author

xareelee commented Aug 3, 2017

@Birowsky sorry, I'm working in ofo (China's biggest bike-sharing startup) and I'm too busy in this year.

@Birowsky
Copy link

Birowsky commented Aug 4, 2017

@xareelee no problemo, have fun mate!

@stubailo
Copy link

stubailo commented Aug 22, 2017

BTW there's also this implementation by @DxCx: https://www.npmjs.com/package/graphql-rxjs

@leebyron
Copy link
Contributor

leebyron commented Dec 1, 2017

For repo maintenance I'm going to close this issue. I'm happy to see the experimentation of a reactive executor in related github projects!

@leebyron leebyron closed this Dec 1, 2017
@alexstrat
Copy link

For anyone around here, I stumbled upon this very promising implementation and amazing work: https://github.com/mesosphere/reactive-graphql

👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants