fix: batch delegation collapses two requests with different arguments into a single memoized loader #5189
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.
馃毃 IMPORTANT: Please do not create a Pull Request without creating an issue first.
Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of the pull request.
Description
When using
batchDelegateToSchema
, the cacheKey for fetching a memoized loader is created using either the requested field name (for fields returning scalars) or the field name + selection set (for fields returning composites). When using this in conjunction with non-key arguments (for instance viaargsFromKeys
), those arguments do not effect the cacheKey. Thus, delegating the same field multiple times with different arguments only ever result in one delegation.The proposed fix changes the cacheKey construction. Instead of using only the
selectionSet
of a composite field node, it uses the entire field node (with the alias redacted). This goes for composite fields as well as scalar fields since both of them could be delegating with non-key arguments.It seems to solve the issue (see
cacheByArguments.test.ts
for a test that fails using the old cacheKey construction method, that now passes). However, there might be nuances to delegation that I am missing (for instance, when would there ever be more than one field node infieldNodes
?).Please have a look and see if my proposal makes sense to you.
Related to #5188
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Environment:
@graphql-tools/batch-delegate
: 8.4.25Checklist: