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

feat: export execution algorithm implementation as Executor class #3193

Closed
wants to merge 21 commits into from

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Jun 21, 2021

Depends on #3196.

@yaacovCR yaacovCR force-pushed the aggregate-error branch 2 times, most recently from 7611c3a to f66639a Compare June 22, 2021 09:15
@yaacovCR yaacovCR marked this pull request as draft June 22, 2021 12:11
@yaacovCR yaacovCR changed the base branch from aggregate-error to throw-on-build-error June 22, 2021 18:33
@yaacovCR yaacovCR force-pushed the refactor-execution branch 2 times, most recently from 6ca3878 to 58ba402 Compare June 22, 2021 19:16
@yaacovCR yaacovCR marked this pull request as ready for review June 22, 2021 19:31
@yaacovCR
Copy link
Contributor Author

#3185 has now been split into the following PR stack: #3192, #3195, #3196, and this PR: #3193


Summary:

#3192 introduces the GraphQLAggregateError that forms an independent feature for reporting multiple errors from a resolver, but also allows the buildExecutionContext method to always return a valid ExecutionContext, throwing when impossible, which allows the method to be used in the eventual Executor class constructor. The handleRawError method is introduced, which recognizes that errors may be aggregated.

#3195 moves the subscription code into the execution module -- at long last! -- bringing the reference implementation in line with the spec. Subscriptions within the spec are put on equal footing as queries and mutations, all three being operations that are targets for "execution," with the different execution algorithms further elaborated on within the "Execution" section of the spec. Code reuse is employed to good effect; for example, error handling for subscriptions is streamlined using the
same handleRawError method.

#3196 streamlines the buildExecutionContext function and related execution setup code to prepare for integration into the eventual Executor class constructor.

#3193 moves all of the "execution algorithm" functions to the Executor class. execute and subscribe are left as thin wrappers around the Executor class that construct an executor object and call executor.executeQueryOrMutation or executor.executeSubscription, respectively.


The subscribe function has been retained because of its different method signature/return type. We could consider introducing query and mutate functions to eventually replace execute so as to properly keep the operations on equal footing.

Thoughts?

@yaacovCR
Copy link
Contributor Author

Of important note -- the Executor class is exported, which was the whole reason for this refactoring (!!!). The class is marked as internal, with a comment/warning similar to the Parser class as suggested by @IvanGoncharov (see #3163 (comment)).

@yaacovCR
Copy link
Contributor Author

A final (?) thought -- the createSourceEventStream function used to be exported separately, which is no longer necessary, as it is included within the Executor class. This export would not seem to have to be covered by the warning attached to the class, but I am not quite sure how to indicate that and/or how to manage that within the code base. I think the code as is may be just as fine?

Suggestions, thoughts, ideas, conversation....all welcome.

@yaacovCR yaacovCR force-pushed the refactor-execution branch 2 times, most recently from 173a790 to 176ea26 Compare June 22, 2021 21:35
@yaacovCR yaacovCR changed the title refactor: execution functions into Executor class feat: export execution algorithm implementation as Executor class Jun 22, 2021
src/jsutils/memoize2.ts Outdated Show resolved Hide resolved
@maraisr
Copy link

maraisr commented Aug 26, 2021

Wow this is so nice @yaacovCR 🙏🏻

Just dropping my 2 cents here; feel this is a really strong move for graphql. It'll allow consumers of this library to iterate much further in isolation with new and exciting features, that eventuate in upstream work. It'll enable defer/stream to become first-class in those areas, where the consumers understand the risk and accept it — without graphql committing to an api.

Completely opening up the internal Executor allows consumers to extend this and overload with their own functionality, which could lead to some exciting experiments and help accelerate adoption in communities who require a niche.

@IvanGoncharov please turn this into a reality for me <3

All operations, including subscription operations, are covered by the execution section of the GraphQL spec
...and executeQueryOrMutationRootFields

to prepare for integration of executeSubscription
this function currently does not throw GraphQLErrors -- it is not within the parallel try block within execute
to executeSubscriptionRootField
to operate only on ExecutionContext
-- In at least a few cases, destructuring assignment from exeContext can improve code readability.
-- Overrides to exeContext can be set using object spread syntax.
Instead of possibly retuning an array of GraphQLErrors, buildExecutionContext can throw a GraphQLAggregateError to report any encountered errors.
only called by buildExecutionContext
This class is exported only to assist people in implementing their own executors without duplicating too much code and should be used only as last resort for cases such as experimental syntax or if certain features could not be contributed upstream.

It is still part of the internal API and is versioned, so any changes to it are never considered breaking changes. If you still need to support multiple versions of the library, please use the `versionInfo` variable for version detection.
ExecutorArgs and ExecutionContext
@yaacovCR yaacovCR force-pushed the throw-on-build-error branch 5 times, most recently from 9cab1b5 to ec09843 Compare October 6, 2021 21:49
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Oct 7, 2021

Closed this in favor of PR stack 3293 => 3302.

@yaacovCR yaacovCR closed this Oct 7, 2021
@yaacovCR yaacovCR deleted the refactor-execution branch October 7, 2021 21:23
@yaacovCR
Copy link
Contributor Author

@maraisr

This is now available for experimentation at:
https://www.npmjs.com/package/graphql-executor
https://github.com/yaacovCR/graphql-executor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants