Define 'execution' as in 'before execution begins' #1174
Conversation
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
leebyron
added
the
✏️ Editorial
Reintroduces #894 atop latest main Co-authored-by: Benjie <benjie@jemjie.com>
leebyron
force-pushed
the
lee/before-execution-begins-note
branch
from
a117f4e to
1be44d0
Compare
| @@ -39,27 +39,44 @@ and have no effect on the observable execution, validation, or response of a | |||
| GraphQL document. Descriptions and comments on executable documents MAY be used | |||
| for non-observable purposes, such as logging and other developer tools. | |||
|
|
|||
| ## Executing Requests | |||
| ## Processing Requests | |||
There was a problem hiding this comment.
I think the intermixing of "execute" and "process" is ultimately more confusing than helpful, despite it being technically more accurate. I think it's the result of the topline section being "Execution" and then within we define execution as a sub-step of "Processing". It's a much larger change to retitle the entire section.
I wonder if instead we could define phases of execution?
| Given this information, the result of {ExecuteRequest(schema, document, | ||
| operationName, variableValues, initialValue)} produces the response, to be | ||
| formatted according to the Response section below. | ||
| Given this information, the result of {Request(schema, document, operationName, |
There was a problem hiding this comment.
Lets think of how we can keep the algorithm names with action verbs. Ideally this has an "execute" or "process" or something here.
Considering the prose above describes this as "request via execution" then "execute request" does seem like what a reader would expect
| rules should be executed. If validation errors are known, they should be | ||
| reported in the list of "errors" in the response and the request must fail | ||
| without execution. | ||
| As explained in the Validation section, only operations from documents which |
| that maps an underlying _source stream_ to a returned _response stream_. | ||
|
|
||
| Subscribe(subscription, schema, variableValues, initialValue): | ||
| Subscription(subscription, schema, variableValues, initialValue): |
There was a problem hiding this comment.
This one is admittedly the weirdest. Regardless of how we net-out on naming, we should rethink the terms used for the phases of subscription
| Note: An error raised before _execution_ begins will typically be a _request | ||
| error_, and once _execution_ begins will typically be an _execution error_. | ||
|
|
||
| :: We define _execution_ as the process of executing the operation's _root |
There was a problem hiding this comment.
I think here is where we could define "during execution" with something more specific to avoid the overloaded term.
These names might not be ideal, but it seems like we have two phases to execution.
-
First there is a pre-execution preparation step where we coerce variables and ensure we have a valid type and object to begin execution with
-
Then there is the actual collection and execution of fields (though even here it's a bit arbitrary that "execution" starts as we begin collecting fields not when we start the first
ExecuteCollectedFields. Perhaps we could call this phase "field execution" or "selection execution"?
I appreciate what we're trying to do here is more clearly define what scenarios lead to an error result with no data entry vs one with a partial data entry. We started with "field error" -- which while unclear for other reasons -- at least evaded this issue. They were clearly errors which occurred at a per-field level. Maybe we just need to do a half-step back to that?
|
TL;DR: we already have a section "Executing Operations"; let's simply rename "execution" to "operation execution" where appropriate - WDYT? @leebyron I've merged these changes back into #894 1 and have then2:
All in all I think this makes the spec text a lot clearer without having to rename any algorithms or introduce different verbs. The only other change that we might want to consider is renaming Please let me know if you're happy with this direction; if so I'll go over it more carefully to ensure it's consistent (in particular I think I might need to update the text of the subscriptions section). Footnotes
|
|
Oh amazing - thanks for getting the original one reopened, I couldn't figure out how to do that. I can close out this copy-cat PR and pick up discussion in the other |
|
Haha you did all the work; I just did |
|
(And to reopen I had to restore the branch it was based on, then change the PR target to main.) |
Reintroduces #894 atop latest main