Skip to content
This repository has been archived by the owner on Apr 14, 2023. It is now read-only.

WIP: Support for new graphql-js subscriptions support #122

Closed
wants to merge 7 commits into from

Conversation

Urigo
Copy link
Contributor

@Urigo Urigo commented May 9, 2017

Background

Based on graphql-js with support for subscriptions (graphql/graphql-js#846) with AsyncIterator.

This PR is based on @mistic PR: #108 and it will be merged along with his changes and the new package's API.

Changes

The main changes are support for subscribe and execute directly from graphql-js package, and manage the subscription/query/mutation (s).

Also, transformation between the AsyncIterator returned from graphql-js to local implementation of Observable and publish the result over the socket.

Usage Example

With the new graphql-js package, the usage with this transport is as follow:

import { subscribe, execute } from 'graphql';
import { SubscriptionServer } from 'subscriptions-transport-ws';
import schema from './schema';

  new SubscriptionServer(
    {
      schema,
      subscribe,
      execute,
    });

mistic and others added 7 commits April 13, 2017 20:41
…s both on client and server side using websockets as transport layer to be used on a new package called graphql-transport-ws.

fix(NA): fixed path of new files.
feat(NA): applied changes according pull request review..

feat(NA): added static method for instance creation.

refact(NA): change the class names back to subscriptionserver and subscriptionclient.

feat(NA): applied changes according pull request review.

feat(NA): added static method for instance creation. fix(NA): fixed error thrown when we try to print and query its already a string.

refact(NA): change the class names back to subscriptionserver and subscriptionclient.

feat(NA): added new helper function to fully extend network interface with ws transport. fix(NA): built legacy messages on server side and other bad object accesses.

feat(NA): enclose wsclient creation inside addGraphQLWS function.

fix(NA): removed useless addGraphQLWS function.

feat(NA): added legacy support to the old server protocol.
feat(NA): added subscription function request.

fix(NA): fixed unsubscribe function.

feat(NA): added constructor verification for executor and schema.

chore(NA): added correct comment inside on message function.

feat(NA): added initial filtering for requests.

feat(NA): added initial filtering for requests.

fix(NA): changed way to store unsubscribe function.

fix(NA): removed contextValue from serverOptions.

fix(NA): correct warn comment when using legacy subscription manager,

chore(NA): Implement Adapter pattern for executor

refact(NA): refactor on ExecuteAdapters. feat(NA): added code to support formatError and formatResponse.

fix(NA): fix way we invoke format functions.

fix(NA): small changes on error creation and executor/subscriptionManager check on loadExecutor.

fix(NA): fix bad usage of base params on execute.

feat(NA): updated ws dependency to the last version.

fix(NA):  overrideDefaultErrorType might be undefined.

fix(NA): support for legacy handler.

fix(NA): fixed const and payload errors.
@Urigo Urigo requested review from glasser, stubailo, NeoPhi and helfer and removed request for glasser May 9, 2017 18:49
rootValue?: any,
contextValue?: any,
variableValues?: { [key: string]: any },
operationName?: string,) => Promise<ExecutionResult>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing definition for SubscribeFunction

src/server.ts Outdated
operationName?: string,
) => Promise<ExecutionResult>;

export interface Executor {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executor shouldn't be remove, but subscribe should be added to it.

src/server.ts Outdated
operationName?: string,
) => ({
variableValues?: { [key: string]: any },
operationName?: string,) => ({
subscribe: (observer) => {
if (ExecuteAdapters.isASubscriptionRequest(document, operationName)) {
observer.error(new Error('Subscriptions are not supported'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should replace this with running subscribe, also pass subscribe (optional) to this funtion.
only if subscribe is not defined, throw this error

src/server.ts Outdated
};
},
});
}

public static executeFromAsyncIteratorFunction(handlerFn: Function, execute: Function): ExecuteReactiveFunction {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this is how to call subscribe and convert to observable :)

src/server.ts Outdated
this.execute = ExecuteAdapters.executeFromSubscriptionManager(subscriptionManager);
} else if ( executor.executeReactive ) {
this.execute = executor.executeReactive.bind(executor);
} else if (subscribe) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

executeReactive shouldn't be removed.. :\

@mistic
Copy link
Contributor

mistic commented May 12, 2017

@Urigo I think u can close this PR. I've pushed your changes into my PR accordingly with @DxCx comments :)

@Urigo Urigo closed this May 14, 2017
@dotansimha dotansimha deleted the feature/new-graphql branch June 12, 2017 08:33
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.

3 participants