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: Rename resolveField function and update comments #3159

Merged
merged 3 commits into from Jun 8, 2021

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Jun 4, 2021

The function implements the "Executing Field" section of the spec and should be called executeField rather than resolveField to reduce ambiguity.

Motivation: (1) correctness and (2) lays the groundwork for -- possibly -- providing a custom function for fields to optimize their execution, say, by not calling getArgumentValues if the arguments are passed unparsed to some external graphql service, or what have you.

Right now, if we would add a custom execute function, we would have to have a code block within a function called resolveField that calls the custom execute. Resolving is a part of executing, and so there is a semantic clash that could get pretty confusing (at least for me).

spec does not have read/write modes. although write is a reference to
mutations, seems proper to not introduce new concepts within the code
@IvanGoncharov IvanGoncharov added the PR: polish 💅 PR doesn't change public API or any observed behaviour label Jun 8, 2021
@IvanGoncharov IvanGoncharov changed the title Rename resolveField function execute: Rename resolveField function Jun 8, 2021
@IvanGoncharov IvanGoncharov changed the title execute: Rename resolveField function execute: Rename resolveField function and update comments Jun 8, 2021
@IvanGoncharov IvanGoncharov merged commit dc2a3eb into graphql:main Jun 8, 2021
@IvanGoncharov
Copy link
Member

@yaacovCR Thanks a lot, especially for doing small commits with meaningful descriptions 👍

Motivation: (1) correctness

#1 is totally enough for accepting any changes like this.
Moreover, as a reference implementation, we should always keep implementation in sync with a spec.

@yaacovCR yaacovCR deleted the rename-resolve-field branch June 8, 2021 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: polish 💅 PR doesn't change public API or any observed behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants