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

refactor: subscribe: introduce buildPerEventExecutionContext #3639

Merged
merged 3 commits into from
Jun 21, 2022

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Jun 12, 2022

= introduces buildPerEventExecutionContext that creates an ExecutionContext for each subscribe event from the original ExecutionContext used to create the event stream
= subscribe now directly builds the ExecutionContext instead of relying on createSourceEventStream
= introduces createSourceEventStreamImpl and executeImpl functions that operate on the built ExecutionContext rather the ExecutionArgs
= subscribe calls the createSourceEventStreamImpl function on the original context and eventuallys calls executeImpl on the per event context created by buildEventExecutionContext.

Motivation:

  1. remove unnecessary buildExecutionContext call, reducing duplicate work
  2. paves the way for easily enhancing the buildPerEventExecutionContext to add a new perEventContextFactory argument to augment the context argument passed to resolvers as need per event.

depends on #3654

@netlify
Copy link

netlify bot commented Jun 12, 2022

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit d51c393
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/62b2145983582e0008433bc8
😎 Deploy Preview https://deploy-preview-3639--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 save-context branch 2 times, most recently from d566dfe to c0a2b54 Compare June 12, 2022 19:44
@yaacovCR yaacovCR requested a review from a team June 12, 2022 20:43
@yaacovCR yaacovCR added the PR: polish 💅 PR doesn't change public API or any observed behaviour label Jun 12, 2022
@yaacovCR
Copy link
Contributor Author

Not the best diagrams (fault my own) screenshotted from yEd:

Original:

original

New:

new

Note that per event, we are now no longer re-performing the portions of BuildExecutionContext that do not need to be reperformed.

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Jun 14, 2022

Note that these changes do imply that the *Impl functions could be methods on an Execution Instance or the need for some other abstraction.

But that is already implied by the passing of ExecutionContext to the majority of functions in the file.

These functions may be renamed/reorganized in later refactoring!

@yaacovCR yaacovCR added PR: bug fix 🐞 requires increase of "patch" version number and removed PR: polish 💅 PR doesn't change public API or any observed behaviour labels Jun 14, 2022
@yaacovCR
Copy link
Contributor Author

Offline, @IvanGoncharov added a few notes:

  1. The spec also indicates that execution context should be built only once for subscriptions in that coercion of variables, for instance, is described only once, not per event.
  2. At the same time, when CreateSorceEventStream and ExecuteSubscriptionEvent are managed by two separate services, as suggested by the spec, coercion will end up happening per event. This PR cannot, should not, and does not change that, because the provided implementation of the full subscription operation utilizes only a single service, exposing CreateSourceEventStream and Execute to do so.

@yaacovCR
Copy link
Contributor Author

I rebased this on top of the simpler changes introduced in #3654

Copy link
Member

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

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

This change solves the real issue.

I don't like some of the solutions here like "executeImpl" or "per even context", but I can't propose any other solution that doesn't involve bigger refactoring.

P.S. I'm working on such bigger refactoring in parallel

yaacovCR added 3 commits June 21, 2022 21:56
= introduces `buildPerEventExecutionContext` that creates an `ExecutionContext` for each subscribe event from the original `ExecutionContext` used to create the event stream
= `subscribe` now directly builds the `ExecutionContext` instead of relying on `createSourceEventStream`
= introduces `createSourceEventStreamImpl` and `executeImpl` functions that operate on the built `ExecutionContext` rather the `ExecutionArgs`
= `subscribe` calls the `createSourceEventStreamImpl` function on the original context and eventuallys calls `executeImpl` on the per event context created by `buildEventExecutionContext`.

Motivation:
1. remove unnecessary `buildExecutionContext` call, reducing duplicate work
2. paves the way for easily enhancing the `buildPerEventExecutionContext` to add a new `perEventContextFactory` argument to augment the context argument passed to resolvers as need per event.

depends on graphql#3638
...now that subscribe and createSourceEventStream both call buildExecutionContext, build errors must be tested separately
@yaacovCR yaacovCR merged commit c1fe951 into graphql:main Jun 21, 2022
@yaacovCR yaacovCR deleted the save-context branch June 21, 2022 18:59
yaacovCR added a commit that referenced this pull request Jun 21, 2022
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 #3639 (review)), but in the meantime, this consolidates the common pattern without any breaking changes.
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Jun 22, 2022
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.
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Jun 24, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: bug fix 🐞 requires increase of "patch" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants