Intial refactoring to work over observables instead of promises

[DxCx]

First Implementation for #501 .
NOTE: Promises still works, this change does not break any existing usage.

TODO:

• Get CR Notes.
• Update any documentation? Which?
• Replace Observables Library? set it to peer instead of dependency? pending a decision on issues.
• Reactive Execution:
  • remove TODO and enable reactive execution.
  • write tests with reactive execution.
  • make sure to take(1) when non-reactive request executes and observable is resolved.

[ghost]

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 cla@fb.com. Thanks!

[ghost]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[DxCx]

Hi @leebyron can you please review this?
I'm very eager to hear your thoughts about this :)

[leebyron]

While I'm excited to support fields that return Observable values, I don't support this approach.

First, we shouldn't take a dependency on a large library like rxjs, either directly or as a peer dependency. Second, we should minimize the set of changes to the core executor. Third, we still need to support Promises/Thenable as the most common use case.

[leebyron]

I'm in favor of continued experimentation in this area though. This is closely aligned with some other earlier experiments to add @LiVe to GraphQL.

[DxCx]

Hi @leebyron thanks for the quick response :)
I agree about the rxjs dependency, that's why i've opened it under discuession, and pending to hear what do you think should be supported..
also, if you will note, i did the minimal change to run over observables rather then promises.
then, in the end of the pipe, execute converts the observable into a promise, so no interface is broken.
and promises are still supported.

[leebyron]

Another question is what we actually want to see from a reactive GraphQL executor. We probably do not want to re-execute fields that are likely to have not changed and the output should be a series of diffs to send over the network rather than a series of whole values

[DxCx]

hehe, seems that we are aligned, just released today rxjs-diff-operator which is exactly for that purpose.

the update logic is same as the old one,
so basically, static values will not be re-executed,
promises are Observables.fromPromise(1) which is observable of 1 value, so they won't re-execute either.
the only re-execution that may occur is when the subscription resolver returns an Observable,
then for each value emitted (next called by the observer), the ObjectType resolvers will be updated with a new root value from the subscription, however, this is the expected behavior as i see it.
Also note that all the existing tests are passing with no modification.

[DxCx]

hi @leebyron, eventually i decided to create a seperated npm (graphql-rxjs) package for this for the moment being.
this way i can experiment it myself,
i'm planing on creating example app that uses it so you can experiment yourself.

[stubailo]

Really excited to see where this goes. I think observables in the resolver return can be a neat way to do subscriptions as well.

[DxCx]

hi @leebyron,

i have a server side example (getting graphiql with subscription over observables)
i thought i posted it a while ago and now i see that i didn't :(

so, if you want to take a look,
https://github.com/DxCx/webpack-apollo-server/tree/rxjs-subscriptions
specifically, on main.ts i define a channel (https://github.com/DxCx/webpack-apollo-server/blob/rxjs-subscriptions/src/main.ts#L15)
and then i use it inside a resolver (https://github.com/DxCx/webpack-apollo-server/blob/rxjs-subscriptions/src/schema/modules/subscription.ts#L10)

if you want to really play with it, just clone it (note this is rxjs-subscriptions branch), and then npm install & npm start.
then you will have a graphiql interface with the subscription on port 3000.

Other then that, i'm not sure what are you against in here,
are you against for the general concept of using Observables along with promises? (which feels much more intuitive versus the pub/sub solution found today)
or for the dependency of rxjs?

also, once this is resolved i would be happy to work on the @defer/@live/@stream directives as well as with observables it's very easy to implement.

[DxCx]

@leebyron can you please reply?

[leebyron]

Hey @DxCx, sorry for the delayed response.

Since this library primarily serves to be a production node server and reference implementation of the spec, I don't think it's the appropriate place for this kind of experimentation for fear that other implementations would see it as part of the direction of GraphQL, and this is not yet what we'd like to do.

Notably - it's important that the execution code align to the spec to be a good reference implementation.

I do think it's a great idea to experiment with observable-based resolvers, it's an exciting idea that deserves to be explored - but I think that exploration deserves it's own repository which can move quickly and experiment with different ideas without worrying about breaking core users.

[leebyron]

I'm going to close this PR since we don't want to pursue this in this repository - I'm looking forward to seeing your work as a separate package.

[DxCx]

hey @leebyron
after seen the work on subscription i was thinking,
if i'll rewrite this using AsyncIterator instead of Observables, is that something you will accept?

i mean, i want to introduce some execute function (executeReactive?) which is able to return more then one result potentially.

[smkhalsa]

Hey @leebyron,

Now that AsyncIterator has reached Stage 4, I'd love to hear your thoughts on @DxCx 's proposal above to add AsyncIterator support to the execute function. The ability to return multiple results from the execute function would be a huge step forward for live queries. Thanks!

[leebyron]

A live query would need a separate core executor. It might use some parts of the algorithm which are the same, however the live executor will have different concerns, so we should not convert the existing executor to be a live executor. I agree that for a live executor, AsyncIterators will be an important implementation detail, however I’d recommend accepting either AIs or Observables from resolver functions

…

On Sat, Feb 17, 2018 at 11:18 AM Sat Mandir S. Khalsa < ***@***.***> wrote: Hey @leebyron <https://github.com/leebyron>, Now that AsyncIterator has reached Stage 4 <tc39/proposals@a41dccc>, I'd love to hear your thoughts on @DxCx <https://github.com/dxcx> 's proposal above to add AsyncIterator support to the execute function. The ability to return multiple results from the execute function would be a huge step forward for live queries. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#502 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADD0pIZEQDOK5Xm8hG8HlbkHQJEbPBfks5tVvu8gaJpZM4KFbHW> .

[smkhalsa]

@leebyron That makes sense. I believe that @DxCx 's graphql-rxjs takes this approach, with a separate executeReactive executor.

Would you consider a PR for adding reactive executors to the library at this time? Thanks again!

[leebyron]

At this point I think it makes sense for add on libraries to work out different approaches. If one approach becomes robust to all use cases we could consider merging it into this library

…

On Sat, Feb 17, 2018 at 10:18 AM Sat Mandir S. Khalsa < ***@***.***> wrote: @leebyron <https://github.com/leebyron> That makes sense. I believe that @DxCx <https://github.com/dxcx> 's graphql-rxjs <https://github.com/DxCx/graphql-rxjs> takes this approach, with a separate executeReactive executor. Would you consider a PR for adding reactive executors to the library at this time? Thanks again! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#502 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADD0qIn6tej-PMBb9tcYQudSC40Kfwjks5tVxfsgaJpZM4KFbHW> .