Publish API should be removed (me thinks)

[gluck]

How one want to publish shouldn't matter to the SubscriptionManager, as the correct way to publish messages is through the onMessage callback.
I think the publish methods on PubSub and SubscriptionManager are confusing, as you can implement a correct (for now) server without them.

E.g. I implemented a PubSub based on Observables:

    pubsub: {
      subscribe(triggerName, onMessage, channelOptions) {
        return Promise.resolve(channelOptions.resolve$.subscribe(onMessage, console.error))
      },
      unsubscribe(subscription) {
        subscription.unsubscribe()
      }
    },

No need for publish (and I wouldn't know how to implement it if I wanted to).

It's fine for the default in-memory-event-based-pubsub-implementation to have a publish method, but it shouldn't be in the interface or wrapped in SubscriptionManager.

Thx.

[stubailo]

I think you're right - this also came up in #13

[gluck]

I can PR that if you'd like, either by dropping the method entirely or dropping its definition/documentation/uses but keeping it (with a console warning) for backward (do we care at this stage ?).

Thx

[helfer]

Yeah, a PR would be great! Thanks @gluck

[DxCx]

This is not pubsub based on obsevables im sorry
You are replacing one broken api with a mutation of the existing one.

If you want observable based api, then a resolver should return an observable.

[helfer]

@DxCx We're interested in improving the current implementation, and rewriting graphql-js isn't in scope. What we're doing is very closely aligned with what FB does internally, and I think it makes sense to keep it that way for now.

[gluck]

To me, Observables are a neat match for pubsub, Rx Subject concept is very much that.
That being said my graphQL resolvers are also Observables (to which I apply .take(1).toPromise() before passing them to graphql-js).

If you suggest (query) schema resolvers (as observables) can be leveraged for realtime updates (@live ?), I'm interested, because I haven't found how to do that.

[DxCx]

then take a look at this: https://github.com/DxCx/webpack-apollo-server/blob/rxjs-subscriptions/src/schema/modules/subscription.ts#L10
note that clockSource is Observable (https://github.com/DxCx/webpack-apollo-server/blob/rxjs-subscriptions/src/main.ts#L15)

graphql-rxjs is a modified graphql library which has observables support,
at the moment i'm working to provide a full example (server-client) usecase (although i dont have the time i want those days to progress as i want :() then i believe it will be merged to offical graphql package.

[thebigredgeek]

@gluck Yeah can we please NOT make Apollo or any of it's GraphQL componentry dependent on RxJS? That would create a lot of unnecessary overhead on the client to load such a massive library that is only going to be used in a couple of places. I personally think that it's overengineering to do so. Having a really simple API for pub/sub seems fine to me. If people wanna use Rx in their apps they can just create subjects or observables and wrap APIs with them. I don't think mandating usage is a great idea.

[gluck]

@thebigredgeek I wasn't advocating for rxjs dependency, and I agree it would be a bad idea for a core library.
Though you can support ES7 observables along with promises with no extra dependency (duck typing), and that's usually a good idea IMO, e.g. That's what graphIQL does.

[thebigredgeek]

Awesome. Sorry I think my typo of putting of not putting NOT in my first sentence might have caused this to come across as edgy, which was not intended. Just edited

[dotansimha]

@gluck
A lot has changed since this issue, and now SubscriptionsManager is deprecated, and the PubSub no longer attached to the subscriptions handler.