feat: only wrap affected resolver functions by using the 'onSchemaChange' hook with 'mapSchema' instead of the 'onResolverCalled' hook. #4760
Conversation
…nge' hook with 'mapSchema' instead of the 'onResolverCalled' hook.
| info.variableValues | ||
| ) || {} | ||
|
|
||
| getDirectiveValues(directive, { directives: [directiveNode] }) || {} |
There was a problem hiding this comment.
Not that it seems info.variableValues was not relevant at all, since we are looking at schema directives, not operation directives.
| if (isPromise(result)) { | ||
| return result.then(() => | ||
| originalResolve(root, args, context, info) | ||
| ) |
There was a problem hiding this comment.
This is a small performance optimization for avoiding making stuff async where not needed
There was a problem hiding this comment.
@yaacovCR also created this cool micro library, it could be used instead.
| if (schema.extensions?.[didMapSchemaSymbol] === true) { | ||
| return | ||
| } | ||
| const transformedSchema = wrapAffectedResolvers(schema, options) | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore | ||
| transformedSchema.extensions = { | ||
| ...schema.extensions, | ||
| [didMapSchemaSymbol]: true, | ||
| } | ||
| replaceSchema(transformedSchema) |
There was a problem hiding this comment.
Since for every replaceSchema call the onSchemaChange hook of all other plugins is called we need a way to track whether a transform has already been applied for preventing an infinite loop. The schema extensions are perfect for this!
| async onResolverCalled({ args, root, context, info }) { | ||
| const directiveNode = getDirectiveByName(info, options.name) | ||
| ): GraphQLSchema { | ||
| return mapSchema(schema, { |
There was a problem hiding this comment.
See https://www.graphql-tools.com/docs/schema-directives#implementing-schema-directives for more context
| * Currently graphql-js extensions typings are limited to string keys. | ||
| * We are using symbols as each useRedwoodDirective plugin instance should use its own unique symbol. |
There was a problem hiding this comment.
Created graphql/graphql-js#3511 to address this.
|
Nice! Also, how is this possible?!? I feel like you've been in the Redwood PRs for so very long now 🤔
|
|
@thedavidprice haha, so far I only talked a lot 😆! Now I am finally getting my own hands dirty 🎉 |
use ts-expect-error so we can remove this ignore once the issue is resolved in graphql :)
|
@n1ru4l what a wonderful contribution from a first time contributor 😉 I very much appreciate your help here, everything seems to work perfectly. I've read your blog posts on this about 5 times now, I still struggle with it 🤣 - really couldn't have done this without the Guild's help. Going to wait for smoke tests to pass then merge! |
As discussed with @dthyresson this is my PR for improving the performance of the
useRedwoodDirectivelogic.