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: move where we call field.coerce_result. #166

Draft
wants to merge 1 commit into
base: myron/improve-perf/change-resolve-interface
Choose a base branch
from

Conversation

myronmarston
Copy link
Collaborator

Previously, we called it from GarphQLAdapter#call, applying it to all resolved fields. However, we only need to use it in the aggregation GroupedBy resolver, where we get DayOfWeek enum values that we may need to coerce. There are no other cases where it is needed.

This allows us to simplify GarphQLAdapter#call so that it now only does two things:

  • Identifies the resolver to dispatch to.
  • Dispatches to that resolver.

This will allow us to further optimize by leveraging built-in functionality of the GraphQL gem to have it dispatch to the appropriate resolver in a more performant manner.

Previously, we called it from `GarphQLAdapter#call`, applying
it to all resolved fields. However, we only need to use it in the
aggregation `GroupedBy` resolver, where we get `DayOfWeek` enum
values that we may need to coerce. There are no other cases where
it is needed.

This allows us to simplify `GarphQLAdapter#call` so that it now only
does two things:

* Identifies the resolver to dispatch to.
* Dispatches to that resolver.

This will allow us to further optimize by leveraging built-in
functionality of the GraphQL gem to have it dispatch to the appropriate
resolver in a more performant manner.
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

2 participants