Properly name the root field execution functions #3294
Conversation
yaacovCR
force-pushed
the
execute-root-fields
branch
from
64917bb
to
ff41b34
Compare
yaacovCR
force-pushed
the
execute-root-fields
branch
from
04f6bd1
to
9d0a0a1
Compare
yaacovCR
force-pushed
the
execute-root-fields
branch
from
9d0a0a1
to
ea6b79e
Compare
|
I’m not sure the overall merge/review strategy, I prepared this stack as individual PRs that could each get merged into main. If there is some other way you want to organize, please let me know how I can help. My only goal is to expedite/facilitate your review and hopefully merging these changes as soon as possible. |
|
That is, each could be merged into main consecutively. Maybe you meant to merge #3295 into main? |
|
I can, sure, but not until several hours from now. Are you able in the meantime to proceed with review? In particular #3294 should be uncontroversial now, as do the rest of them (in my opinion). |
|
@yaacovCR I was looking into |
The codebase should refer to functions that execute the query, mutation, and/or subscription root fields as such, rather than as functions that execute the operations themselves. Executing a query or mutation returns a map of data and errors, while executing the root fields returns the data of the root fields.
yaacovCR
force-pushed
the
execute-root-fields
branch
from
05e2b21
to
fdf3e33
Compare
I rebased everything. It's all ready to review and merge. |
| exeContext: ExecutionContext, | ||
| operation: OperationDefinitionNode, | ||
| rootValue: unknown, | ||
| ): PromiseOrValue<ObjMap<unknown> | null> { |
There was a problem hiding this comment.
Context is a necessary evil, for stuff used deeply in the call chain.
If arguments are used directly we should pass them directly.
There was a problem hiding this comment.
Especially, since a plan is to separate operation execution context later.
| @@ -190,7 +190,7 @@ export async function createSourceEventStream( | |||
| } | |||
| } | |||
|
|
|||
| async function executeSubscription( | |||
| async function executeSubscriptionRootField( | |||
There was a problem hiding this comment.
Let's keep, naming for now.
It's better to refactor them to return proper response once we separate operation execution context.
We can add TODO or FIXME to make it clearer.
Just don't want to rename stuff prematurly.
There was a problem hiding this comment.
See comment above, disagree strongly.
There was a problem hiding this comment.
There is nothing premature about correcting an error, just because you will further improve later, this is internal code refactoring, non breaking changes, you can rererename as often as you want
| */ | ||
| function executeOperation( | ||
| function executeQueryOrMutationRootFields( |
There was a problem hiding this comment.
As discussed previously want to put subscription here.
So let's keep the name.
There was a problem hiding this comment.
The name as now is incorrect, for internal refactoring, better to just improve instead of adding todo to get optimal solution. There is a saying, “perfect is the enemy of good.”
|
As per our offline discussion, I am forking the executor only and will be pushing forward asap with class based Executor over there, in order to jumpstart experimentation. Hopefully whatever works at |
The codebase should refer to functions that execute the query, mutation, and/or subscription root fields as such, rather than as functions that execute the operations themselves. Executing a query or mutation returns a map of data and errors, while executing the root fields returns the completed value of the root fields.
Depends on #3306