From 4a8c0b55686dd80ce0c373fc93b28474167a60af Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Fri, 10 Jun 2022 06:52:35 +0300 Subject: [PATCH 1/5] createSourceEventStream: allow named arguments Deprecates the positional arguments to `createSourceEventStream`, to be removed in the next major version. --- src/execution/__tests__/subscribe-test.ts | 44 +++++++++- src/execution/subscribe.ts | 97 +++++++++++------------ 2 files changed, 88 insertions(+), 53 deletions(-) diff --git a/src/execution/__tests__/subscribe-test.ts b/src/execution/__tests__/subscribe-test.ts index 03e3da2839..1dbdb05e5b 100644 --- a/src/execution/__tests__/subscribe-test.ts +++ b/src/execution/__tests__/subscribe-test.ts @@ -191,7 +191,7 @@ function subscribeWithBadFn( return expectEqualPromisesOrValues( subscribe({ schema, document }), - createSourceEventStream(schema, document), + createSourceEventStream({ schema, document }), ); } @@ -421,6 +421,48 @@ describe('Subscription Initialization Phase', () => { expect(() => subscribe({ schema })).to.throw('Must provide document.'); }); + it('allows positional arguments to createSourceEventStream', () => { + async function* fooGenerator() { + /* c8 ignore next 2 */ + yield { foo: 'FooValue' }; + } + + const schema = new GraphQLSchema({ + query: DummyQueryType, + subscription: new GraphQLObjectType({ + name: 'Subscription', + fields: { + foo: { type: GraphQLString, subscribe: fooGenerator }, + }, + }), + }); + const document = parse('subscription { foo }'); + + const eventStream = createSourceEventStream(schema, document); + assert(isAsyncIterable(eventStream)); + }); + + it('throws an error if document is missing when using positional arguments', async () => { + const schema = new GraphQLSchema({ + query: DummyQueryType, + subscription: new GraphQLObjectType({ + name: 'Subscription', + fields: { + foo: { type: GraphQLString }, + }, + }), + }); + + // @ts-expect-error + expect(() => createSourceEventStream(schema, null)).to.throw( + 'Must provide document.', + ); + + expect(() => createSourceEventStream(schema)).to.throw( + 'Must provide document.', + ); + }); + it('resolves to an error if schema does not support subscriptions', async () => { const schema = new GraphQLSchema({ query: DummyQueryType }); const document = parse('subscription { unknownField }'); diff --git a/src/execution/subscribe.ts b/src/execution/subscribe.ts index 9ff7cd2112..aa45240e3b 100644 --- a/src/execution/subscribe.ts +++ b/src/execution/subscribe.ts @@ -1,4 +1,5 @@ import { inspect } from '../jsutils/inspect'; +import { invariant } from '../jsutils/invariant'; import { isAsyncIterable } from '../jsutils/isAsyncIterable'; import { isPromise } from '../jsutils/isPromise'; import type { Maybe } from '../jsutils/Maybe'; @@ -12,6 +13,7 @@ import type { DocumentNode } from '../language/ast'; import type { GraphQLFieldResolver } from '../type/definition'; import type { GraphQLSchema } from '../type/schema'; +import { isSchema } from '../type/schema'; import { collectFields } from './collectFields'; import type { @@ -54,60 +56,20 @@ export function subscribe( ): PromiseOrValue< AsyncGenerator | ExecutionResult > { - const { - schema, - document, - rootValue, - contextValue, - variableValues, - operationName, - fieldResolver, - subscribeFieldResolver, - } = args; - - const resultOrStream = createSourceEventStream( - schema, - document, - rootValue, - contextValue, - variableValues, - operationName, - subscribeFieldResolver, - ); + const resultOrStream = createSourceEventStream(args); if (isPromise(resultOrStream)) { return resultOrStream.then((resolvedResultOrStream) => - mapSourceToResponse( - schema, - document, - resolvedResultOrStream, - contextValue, - variableValues, - operationName, - fieldResolver, - ), + mapSourceToResponse(resolvedResultOrStream, args), ); } - return mapSourceToResponse( - schema, - document, - resultOrStream, - contextValue, - variableValues, - operationName, - fieldResolver, - ); + return mapSourceToResponse(resultOrStream, args); } function mapSourceToResponse( - schema: GraphQLSchema, - document: DocumentNode, resultOrStream: ExecutionResult | AsyncIterable, - contextValue?: unknown, - variableValues?: Maybe<{ readonly [variable: string]: unknown }>, - operationName?: Maybe, - fieldResolver?: Maybe>, + args: ExecutionArgs, ): PromiseOrValue< AsyncGenerator | ExecutionResult > { @@ -123,13 +85,8 @@ function mapSourceToResponse( // "ExecuteQuery" algorithm, for which `execute` is also used. return mapAsyncIterator(resultOrStream, (payload: unknown) => execute({ - schema, - document, + ...args, rootValue: payload, - contextValue, - variableValues, - operationName, - fieldResolver, }), ); } @@ -163,14 +120,50 @@ function mapSourceToResponse( * "Supporting Subscriptions at Scale" information in the GraphQL specification. */ export function createSourceEventStream( - schema: GraphQLSchema, - document: DocumentNode, + argsOrSchema: ExecutionArgs | GraphQLSchema, + /** Note: document argument is required if positional arguments are used */ + /** @deprecated will be removed in next major version in favor of named arguments */ + document?: DocumentNode, + /** @deprecated will be removed in next major version in favor of named arguments */ rootValue?: unknown, + /** @deprecated will be removed in next major version in favor of named arguments */ contextValue?: unknown, + /** @deprecated will be removed in next major version in favor of named arguments */ variableValues?: Maybe<{ readonly [variable: string]: unknown }>, + /** @deprecated will be removed in next major version in favor of named arguments */ operationName?: Maybe, + /** @deprecated will be removed in next major version in favor of named arguments */ subscribeFieldResolver?: Maybe>, ): PromiseOrValue | ExecutionResult> { + if (isSchema(argsOrSchema)) { + invariant(document != null, 'Must provide document.'); + return createSourceEventStreamImpl({ + schema: argsOrSchema, + document, + rootValue, + contextValue, + variableValues, + operationName, + subscribeFieldResolver, + }); + } + + return createSourceEventStreamImpl(argsOrSchema); +} + +export function createSourceEventStreamImpl( + args: ExecutionArgs, +): PromiseOrValue | ExecutionResult> { + const { + schema, + document, + rootValue, + contextValue, + variableValues, + operationName, + subscribeFieldResolver, + } = args; + // If arguments are missing or incorrectly typed, this is an internal // developer mistake which should throw an early error. assertValidExecutionArguments(schema, document, variableValues); From e8a2a7405c921b549124db8f7a53c241f8b375b2 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Fri, 10 Jun 2022 11:21:09 +0300 Subject: [PATCH 2/5] apply review feedback --- src/execution/__tests__/subscribe-test.ts | 1 + src/execution/subscribe.ts | 20 +++++++++++++------- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/execution/__tests__/subscribe-test.ts b/src/execution/__tests__/subscribe-test.ts index 1dbdb05e5b..6e5168c748 100644 --- a/src/execution/__tests__/subscribe-test.ts +++ b/src/execution/__tests__/subscribe-test.ts @@ -458,6 +458,7 @@ describe('Subscription Initialization Phase', () => { 'Must provide document.', ); + // @ts-expect-error expect(() => createSourceEventStream(schema)).to.throw( 'Must provide document.', ); diff --git a/src/execution/subscribe.ts b/src/execution/subscribe.ts index aa45240e3b..13f3a8647d 100644 --- a/src/execution/subscribe.ts +++ b/src/execution/subscribe.ts @@ -119,20 +119,26 @@ function mapSourceToResponse( * or otherwise separating these two steps. For more on this, see the * "Supporting Subscriptions at Scale" information in the GraphQL specification. */ +export function createSourceEventStream( + args: ExecutionArgs, +): PromiseOrValue | ExecutionResult>; +/** @deprecated will be removed in next major version in favor of named arguments */ +export function createSourceEventStream( + schema: GraphQLSchema, + document: DocumentNode, + rootValue?: unknown, + contextValue?: unknown, + variableValues?: Maybe<{ readonly [variable: string]: unknown }>, + operationName?: Maybe, + subscribeFieldResolver?: Maybe>, +): PromiseOrValue | ExecutionResult>; export function createSourceEventStream( argsOrSchema: ExecutionArgs | GraphQLSchema, - /** Note: document argument is required if positional arguments are used */ - /** @deprecated will be removed in next major version in favor of named arguments */ document?: DocumentNode, - /** @deprecated will be removed in next major version in favor of named arguments */ rootValue?: unknown, - /** @deprecated will be removed in next major version in favor of named arguments */ contextValue?: unknown, - /** @deprecated will be removed in next major version in favor of named arguments */ variableValues?: Maybe<{ readonly [variable: string]: unknown }>, - /** @deprecated will be removed in next major version in favor of named arguments */ operationName?: Maybe, - /** @deprecated will be removed in next major version in favor of named arguments */ subscribeFieldResolver?: Maybe>, ): PromiseOrValue | ExecutionResult> { if (isSchema(argsOrSchema)) { From 120ae79e139db444784f51d66d215f953e2deaf6 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Fri, 10 Jun 2022 11:54:25 +0300 Subject: [PATCH 3/5] apply feedback --- src/execution/__tests__/subscribe-test.ts | 2 +- src/execution/subscribe.ts | 63 ++++++++++++----------- 2 files changed, 34 insertions(+), 31 deletions(-) diff --git a/src/execution/__tests__/subscribe-test.ts b/src/execution/__tests__/subscribe-test.ts index 6e5168c748..5ac09d253f 100644 --- a/src/execution/__tests__/subscribe-test.ts +++ b/src/execution/__tests__/subscribe-test.ts @@ -421,7 +421,7 @@ describe('Subscription Initialization Phase', () => { expect(() => subscribe({ schema })).to.throw('Must provide document.'); }); - it('allows positional arguments to createSourceEventStream', () => { + it('Deprecated: allows positional arguments to createSourceEventStream', () => { async function* fooGenerator() { /* c8 ignore next 2 */ yield { foo: 'FooValue' }; diff --git a/src/execution/subscribe.ts b/src/execution/subscribe.ts index 13f3a8647d..55b72dd5a8 100644 --- a/src/execution/subscribe.ts +++ b/src/execution/subscribe.ts @@ -13,7 +13,6 @@ import type { DocumentNode } from '../language/ast'; import type { GraphQLFieldResolver } from '../type/definition'; import type { GraphQLSchema } from '../type/schema'; -import { isSchema } from '../type/schema'; import { collectFields } from './collectFields'; import type { @@ -91,6 +90,37 @@ function mapSourceToResponse( ); } +type BackwardsCompatibleArgs = + | [options: ExecutionArgs] + | [ + schema: ExecutionArgs['schema'], + document: ExecutionArgs['document'], + rootValue?: ExecutionArgs['rootValue'], + contextValue?: ExecutionArgs['contextValue'], + variableValues?: ExecutionArgs['variableValues'], + operationName?: ExecutionArgs['operationName'], + subscribeFieldResolver?: ExecutionArgs['subscribeFieldResolver'], + ]; + +function toNormalizedArgs(args: BackwardsCompatibleArgs): ExecutionArgs { + const firstArg = args[0]; + if ('document' in firstArg) { + return firstArg; + } + + invariant(args[1] != null, 'Must provide document.'); + + return { + schema: firstArg, + document: args[1], + rootValue: args[2], + contextValue: args[3], + variableValues: args[4], + operationName: args[5], + subscribeFieldResolver: args[6], + }; +} + /** * Implements the "CreateSourceEventStream" algorithm described in the * GraphQL specification, resolving the subscription source event stream. @@ -132,34 +162,7 @@ export function createSourceEventStream( operationName?: Maybe, subscribeFieldResolver?: Maybe>, ): PromiseOrValue | ExecutionResult>; -export function createSourceEventStream( - argsOrSchema: ExecutionArgs | GraphQLSchema, - document?: DocumentNode, - rootValue?: unknown, - contextValue?: unknown, - variableValues?: Maybe<{ readonly [variable: string]: unknown }>, - operationName?: Maybe, - subscribeFieldResolver?: Maybe>, -): PromiseOrValue | ExecutionResult> { - if (isSchema(argsOrSchema)) { - invariant(document != null, 'Must provide document.'); - return createSourceEventStreamImpl({ - schema: argsOrSchema, - document, - rootValue, - contextValue, - variableValues, - operationName, - subscribeFieldResolver, - }); - } - - return createSourceEventStreamImpl(argsOrSchema); -} - -export function createSourceEventStreamImpl( - args: ExecutionArgs, -): PromiseOrValue | ExecutionResult> { +export function createSourceEventStream(...rawArgs: BackwardsCompatibleArgs) { const { schema, document, @@ -168,7 +171,7 @@ export function createSourceEventStreamImpl( variableValues, operationName, subscribeFieldResolver, - } = args; + } = toNormalizedArgs(rawArgs); // If arguments are missing or incorrectly typed, this is an internal // developer mistake which should throw an early error. From c39e0f350feb7f6be8b67f4da98885ced7991ca5 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Fri, 10 Jun 2022 14:24:55 +0300 Subject: [PATCH 4/5] Remove unnecessary invariant in favor of explicit cast See open TS issue https://github.com/microsoft/TypeScript/issues/31613 Marked with FIXME for when TS feature/bug fix lands upstream [This code will likely be removed in v17 prior to the feature landing, but may be retained within v16 for longer.] Similar code "works" at https://github.com/graphql/graphql-js/blob/1f8ba95c662118452bd969c6b26ba4e9050c55da/src/error/GraphQLError.ts#L47 because all the option properties are optional. --- src/execution/subscribe.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/execution/subscribe.ts b/src/execution/subscribe.ts index 55b72dd5a8..74608d7d25 100644 --- a/src/execution/subscribe.ts +++ b/src/execution/subscribe.ts @@ -1,5 +1,4 @@ import { inspect } from '../jsutils/inspect'; -import { invariant } from '../jsutils/invariant'; import { isAsyncIterable } from '../jsutils/isAsyncIterable'; import { isPromise } from '../jsutils/isPromise'; import type { Maybe } from '../jsutils/Maybe'; @@ -108,11 +107,10 @@ function toNormalizedArgs(args: BackwardsCompatibleArgs): ExecutionArgs { return firstArg; } - invariant(args[1] != null, 'Must provide document.'); - return { schema: firstArg, - document: args[1], + // FIXME: when underlying TS bug fixed, see https://github.com/microsoft/TypeScript/issues/31613 + document: args[1] as DocumentNode, rootValue: args[2], contextValue: args[3], variableValues: args[4], From f5ebc6f803c60a1f747aae7c0d585b9f08bc1dbf Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Sun, 12 Jun 2022 16:53:06 +0300 Subject: [PATCH 5/5] add deprecated comment https://github.com/graphql/graphql-js/pull/3635#discussion_r894691771 --- src/execution/__tests__/subscribe-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/execution/__tests__/subscribe-test.ts b/src/execution/__tests__/subscribe-test.ts index 5ac09d253f..b1ca701185 100644 --- a/src/execution/__tests__/subscribe-test.ts +++ b/src/execution/__tests__/subscribe-test.ts @@ -442,7 +442,7 @@ describe('Subscription Initialization Phase', () => { assert(isAsyncIterable(eventStream)); }); - it('throws an error if document is missing when using positional arguments', async () => { + it('Deprecated: throws an error if document is missing when using positional arguments', async () => { const schema = new GraphQLSchema({ query: DummyQueryType, subscription: new GraphQLObjectType({