WIP: Continue work to support new graphql-js

[dotansimha]

Continue #108

As discussed in the previous PR:

• Removed inner Observable implementation, and replace it with AsyncIterator, removingObservable code completely.
• Add support for GraphQL execute return Promise, along with execute that returns AsyncIterator to support reactive data sources.
• Introduce SubscriptionManager to AsyncIterator to support old graphql-subscriptions API.
• Added more unit tests to test support of graphql-js with AsyncIterator
• Apply a fix to support app using upgradeReq (changed since ws@3.0.0).
• Split some of the server code into smaller files.

Still WIP:

• More unit tests (for subscribe executor)

@Urigo @DxCx @mistic

TODO:

• 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
• Update CHANGELOG.md with your change

[Urigo]

@dotansimha awesome!! also adding @NeoPhi for the review

[leebyron]

This seems pretty strange - are you sure this is doing what you expect? The return() and throw() API on AsyncIterator are intended to be called by the consumer, not internally.

Since an AsyncIterator yields Promises, I would expect that it would simply return the wrapped promise once.

I would expect:

import { $$asyncIterator } from 'iterall';

export function createIterableFromPromise<T>(promise: Promise<T>): AsyncIterator<T> {
  let isComplete = false;

  return {
    next() {
      if (!isComplete) {
        isComplete = true;
        return promise.then(value => { value, done: false });
      }
      return Promise.resolve({ value: undefined, done: true });
    },
    return() {
      isComplete = true;
      return Promise.resolve({ value: undefined, done: true });
    },
    throw(e: Error) {
      isComplete = true;
      return Promise.reject(e);
    },
    [$$asyncIterator]() {
      return this;
    },
  };
}

[dotansimha]

Yeah that makes sense. I will change it and added unit tests

[leebyron]

should be return Promise.resolve({ done: true, value: undefined })

[dotansimha]

Fixed!

[leebyron]

A bit confusing to have this call itself - perhaps just return Promise.reject(error)?

[dotansimha]

Fixed! Thanks @leebyron

[leebyron]

If this is attempting to model graphql-js's execute function, that always returns a Promise. Only subscribe returns an AsyncIterator

[dotansimha]

@leebyron
Yeah, the original execute of graphql-js returns a Promise.
I talked to @DxCx and we think that if we allow execute to return AsyncIterator in this package, we can also support Hagai's implementation for reactive graphql (#108 (comment))

[DxCx]

@leebyron this is done to allow execution of reactive directives as well (@live) for example, if one uses vanilla graphQL he will get the standard execute..

[Urigo]

@leebyron I wanted to make sure that anyone who uses this package and expects graohql-js's execute will get exactly that. It almost won't effect you and the docs will be focused on that. But at the same time, if you want to use a separate, experimental package, it won't block you from it.

[leebyron]

Sounds great, just wanted to make sure it wasn't a mistake :)

[DxCx]

why so many casting?

[dotansimha]

Fixed!

[DxCx]

@dotansimha since promise have only 1 result anyway,
shouldn't you return value, done: true on next()
and use return as teardown function (which doesn't do anything since promises are not cancelable)?

[dotansimha]

The return of AsyncIterator should not return a value, only done=false.
If setting done=true while the Promise is resolved, the value ignored and then the iterator is done.

[NeoPhi]

This is all looking great.