perf(gatsby): do not force resolvers to be async #28525
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gatsbot
bot
added
the
status: triage needed
pvdz
added
topic: performance
wardpeet
reviewed
Dec 8, 2020
| any, | ||
| any | ||
| > = function defaultFieldResolver(source, args, context, info) { | ||
| function _defaultFieldResolver(source, args, context, info): any { |
There was a problem hiding this comment.
why not keep const & function on same line? Does _defaultFieldResolver still get correct typings?
There was a problem hiding this comment.
Because then a lint rule of premature usage will trigger, which we do not apply to function declarations.
My attempt failed anyways so I'll change it back and move the whole function instead 🤷
If I throw inside the function and run tests there will be 16 tests that fail where otherwise tests pass. This applies to both return points. |
vladar
reviewed
Dec 8, 2020
vladar
reviewed
Dec 9, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Drop the
asynckeyword from thewrappingResolverthat wraps all resolvers passed on to graphql. This keyword forces the return value to be a promise, injecting tiny delays in each call. Most of the time you wouldn't notice these but at scale they add up.In this case a large site was triggering the link resolver 35 million times. Only 1.5 million times did the wrapped resolver return a promise. And graphql will properly deal with promises vs non-promises. So we can drop the keyword.
The PR also updates the
linkResolverto take the fast path (when target key isidand the resolver is our owndefaultResolver) sync rather than also force it to be async. ThedefaultResolveralready has noasynckeyword.Doing so drops roughly 5-10% from the runtime of the site I was testing with (!). Promises can be expensive. They should put a warning sign around them.