Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

execute: integrate subscriptions and refactor pipeline #3644

Closed
wants to merge 13 commits into from

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Jun 14, 2022

execute no longer runs the query algorithm for subscription operations. Rather, subscription operations are performed, as per the spec. subscribe and createSourceEventStream are removed.

See additional comments below.

@netlify
Copy link

netlify bot commented Jun 14, 2022

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 3f69fb3
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/62bb53b590545b0008a5aa4b
😎 Deploy Preview https://deploy-preview-3644--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

@yaacovCR yaacovCR force-pushed the make-execute-happen branch 4 times, most recently from ce28d68 to dc3b18d Compare June 14, 2022 10:50
@yaacovCR yaacovCR requested a review from a team June 14, 2022 10:51
@yaacovCR yaacovCR added the PR: breaking change 💥 implementation requires increase of "major" version number label Jun 14, 2022
@yaacovCR yaacovCR force-pushed the make-execute-happen branch 8 times, most recently from 393bdfc to eaf786d Compare June 21, 2022 20:36
@yaacovCR yaacovCR force-pushed the make-execute-happen branch 4 times, most recently from cbd99b5 to 6946681 Compare June 22, 2022 19:31
@yaacovCR yaacovCR force-pushed the make-execute-happen branch from a198622 to f18583f Compare June 24, 2022 08:25
yaacovCR added 5 commits June 24, 2022 12:09
The `execute`/`executeImpl` and `createSourceEventStream`/`createSourceEventStreamImpl` functions follow the same basic pattern of building the contet and using it to run a function. This PR extracts that pattern into a separate function.

For good measure, the same pattern in applied to the soon-to-be-deprecated `subscribe` function.

Hheavier refactoring is on the way from @IvanGoncharov (see graphql#3639 (review)), but in the meantime, this consolidates the common pattern without any breaking changes.
...to executeSubscriptionRootField => because that's what it does!
...because that's what it does!

The execution of an operation returns a map of data/errors, not just the data.
`execute` no longer runs the query algorithm for subscription operations. Rather, subscription operations are performed, as per the spec. `subscribe` is deprecated.
to replace the old exported functionality from execute
@yaacovCR yaacovCR force-pushed the make-execute-happen branch from f18583f to 9fc1d93 Compare June 24, 2022 09:14
@yaacovCR yaacovCR mentioned this pull request Jun 24, 2022
yaacovCR added 8 commits June 28, 2022 21:38
separates internal errors array from the remainder of the Execution data.

obviates the need for the buildPerEventExecutionContext function => as described by the spec, all execution info is the same for execution of subscription events except for the root value, which is set as the payload

the separate executionContext variable introduced is similar to the payloadContext that will be necessary to support defer/stream
Advanced clients setting up services that only process subscriptions should be encouraged to create a custom ExecutionInfo object and use the corresponding internal functions that take ExecutionInfo objects rather than raw arguments.

These functions are currently createSourceEventStreamImpl and executeQuery but when createSourceEventStream is removed, reateSourceEventStreamImpl will be renamed to createSourceEventStream
in favor of the corresponding functions that take ExecutionInfo arguments, i.e. createSourceEventStreamImpl and executeQuery.

Note that since createSourceEventStream has been removed, createSourceEventStreamImpl can be renamed to createSourceEventStream, so that the recommended function name has not in fact changed, buf the function now takes an ExecutionInfo argument
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Jun 29, 2022

Now that #3658 suggests that we may be awaiting a serious refactor, this PR has been reworked to represent a least change approach to the existing API. Rather than list the individual steps to the granularity of every renamed function, I will just list broadly what the PR accomplishes, after which I will list what the PR does not do.

  1. updates execute to run the full ExecuteSubscription algorithm rather than just ExecuteSubscriptionEvent, updating the return types of execute and graphql
  2. removes subscribe/createSourceEventStream (these would have to be deprecated in a 16.x release).
  3. introduces DocumentInfo and ExecutionInfo interfaces that advanced clients can use to analyze a document and set up execution parameters. Internal errors variable -- the only necessarily private information required during execution -- is left within ExecutionContext. This is generally similar to the setup within defer/stream with payload context objects, so the separate variable is not much of an issue (in my opinion).
  4. export helpers buildDocumentInfo, coerceVariableValues, as well as buildExecutionInfo so that advanced clients can have some help creating parts or all of the DocumentInfo/ExecutionInfo.
  5. expose helpers executeQuery, executeMutation, executeSubscription, and createSourceEventStream which take ExecutionInfo rather than ExecutionContext objects. These functions return appropriately typed versions, i.e. executeQuery/executeMutation do not ever return streams (until defer/stream) merged.
  6. no need anymore for the newly introduced *Impl functions within execute

What this PR does not do:

  1. compile schemas/documents/field collections/variables/arguments
  2. provide runtime or type checks for the arguments to executeQuery, etc. I.e. executeQuery can only take an ExecutionInfo object with an operation that is of type QUERY (for ExecuteQuery) or SUBSCRIPTION (for ExecuteSubscriptionEvent). executeMutation should only work for info objects containing a MUTATION, and executeSubscription and createSourceEventStream should only work for info objects containing a SUBSCRIPTION. That is because that TS does not (yet?) support discriminated unions via nested properties and so it's not as easy to do without introducing repetitive type guards that drill down into the ExecutionInfo object or an additional somewhat unnecessary property on ExecutionInfo itself. Considering the type safety comes at a runtime expense, I am forgoing this for now.

@yaacovCR
Copy link
Contributor Author

The new exported helpers have not yet been exported from main, and are only available via deep imports. That can/should change once the API is finalized.

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Jun 29, 2022

Ah => I forgot to mention that I have also snuck in here a new argument to allow total customization of the executor used by EXECUTE_SUBSCRIPTION_EVENT.

As well as a default executor:

export const defaultSubscriptionEventExecutor = (
  exeInfo: ExecutionInfo,
  payload: unknown,
): PromiseOrValue<ExecutionResult> =>
  executeQuery({
    ...exeInfo,
    rootValue: payload,
  });

This workflow allows the buildPerEventExecutionContext function to be removed.

@yaacovCR yaacovCR requested review from a team and removed request for a team June 29, 2022 08:16
@yaacovCR yaacovCR changed the title execute: integrate subscriptions execute: integrate subscriptions and refactor pipeline Jul 21, 2022
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Oct 1, 2024

Closing this for now in favor of plan in #4210

@yaacovCR yaacovCR closed this Oct 1, 2024
yaacovCR added a commit that referenced this pull request Oct 1, 2024
to safely export buildExecutionContext() as validateExecutionArgs()

motivation: this will allow us to:

1. export `executeOperation()` and `executeSubscription()` for those who would like to use directly.
2. add a `perEventExecutor` option to `ExecutionArgs` that allows one to pass a custom context for execution of each execution event, with the opportunity to clean up the context on return, a la #2485 and #3071, addressing #894, which would not require re-coercion of variables.

The signature of the `perEventExecutor` option would be:

```ts
type SubscriptionEventExecutor = ( validatedExecutionArgs: ValidatedExecutionArgs): PromiseOrValue<ExecutionResult>
```

rather than:

```ts
type SubscriptionEventExecutor = ( executionArgs: ExecutionArgs): PromiseOrValue<ExecutionResult>
```

This might be a first step to integrating `subscribe()` completely into `execute()` (see: #3644) but is also a reasonable stopping place.
erikwrede pushed a commit to erikwrede/graphql-js that referenced this pull request Oct 17, 2024
to safely export buildExecutionContext() as validateExecutionArgs()

motivation: this will allow us to:

1. export `executeOperation()` and `executeSubscription()` for those who would like to use directly.
2. add a `perEventExecutor` option to `ExecutionArgs` that allows one to pass a custom context for execution of each execution event, with the opportunity to clean up the context on return, a la #2485 and #3071, addressing #894, which would not require re-coercion of variables.

The signature of the `perEventExecutor` option would be:

```ts
type SubscriptionEventExecutor = ( validatedExecutionArgs: ValidatedExecutionArgs): PromiseOrValue<ExecutionResult>
```

rather than:

```ts
type SubscriptionEventExecutor = ( executionArgs: ExecutionArgs): PromiseOrValue<ExecutionResult>
```

This might be a first step to integrating `subscribe()` completely into `execute()` (see: graphql/graphql-js#3644) but is also a reasonable stopping place.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: breaking change 💥 implementation requires increase of "major" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants