fix(graphql): avoid duplicate function when overriding resolvers #9980
Conversation
sundersc
force-pushed
the
duplicate-resolver-override
branch
from
1729559
to
c5b5b79
Compare
Codecov Report
@@ Coverage Diff @@
## master #9980 +/- ##
=======================================
Coverage 54.09% 54.09%
=======================================
Files 837 837
Lines 46374 46374
Branches 9890 9890
=======================================
Hits 25084 25084
Misses 19295 19295
Partials 1995 1995 📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
packages/amplify-graphql-transformer-core/src/transformer-context/resolver.ts
Outdated
Show resolved
Hide resolved
packages/amplify-graphql-transformer-core/src/transformer-context/resolver.ts
Outdated
Show resolved
Hide resolved
| return false; | ||
| } | ||
|
|
||
| for (const slotItem of slotEntires) { |
There was a problem hiding this comment.
Rather than needing a mutable slotIndex can we just call slotEntries.forEach((slotEntry, slotIndex) => { /** your logic below **/ };
There was a problem hiding this comment.
This block requires early termination if match is found. The only way to early exit foreach is using exception, that would look ugly. I have also considered the usage of every() and some(), however for .. of ... seems to be the best fit here. I'm open to suggestions if there is a better way.
There was a problem hiding this comment.
Ahh, I see. I'm fine w/ this as-is in that case.
|
|
||
| for (const slotItem of slotEntires) { | ||
| const [slotItemRequestMappingTemplate, slotItemResponseMappingTemplate] = [ | ||
| (slotItem.requestMappingTemplate as any)?.name ?? '', |
There was a problem hiding this comment.
Is there a specific type we can use here instead of any?
There was a problem hiding this comment.
Unfortunately no. We could alter the class structure we have for storing templates but that would be outside the scope of the change intended here and requires quite a bit of refactoring.
...index-transformer/src/__tests__/__snapshots__/amplify-graphql-index-transformer.test.ts.snap
Outdated
Show resolved
Hide resolved
|
|
||
| // If name matches, then it is an overridden resolver | ||
| if ( | ||
| slotItemRequestMappingTemplate === ((requestMappingTemplate as any)?.name ?? '') |
There was a problem hiding this comment.
nit: can you use $TSAny if there isn't a better alternative?
There was a problem hiding this comment.
$TSAny is defined on amplify-cli-core. Currently, amplify-graphql-transformer-core doesn't have a dependency on amplify-cli-core. We can consider adding something similar to $TSAny type within data package once we split the repository.
|
|
||
| // If name matches, then it is an overridden resolver | ||
| if ( | ||
| slotItemRequestMappingTemplate === ((requestMappingTemplate as any)?.name ?? '') |
There was a problem hiding this comment.
In the case the user only wants to override the request mapping template but not the response VTL, will this logic work?
There was a problem hiding this comment.
Yes. For example, if response VTL is not overridden, (responseMappingTemplate as any)?.name would be undefined. In that case, the slot's response VTL is preserved.
slot.responseMappingTemplate = (responseMappingTemplate as any)?.name
? responseMappingTemplate
: slot.responseMappingTemplate;
| const slot = this.findSlot(slotName, requestMappingTemplate, responseMappingTemplate); | ||
| if (slot) { | ||
| return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I think you can probably simplify this to return this.findSlot() !== undefined
There was a problem hiding this comment.
That was me being lazy 🤣 I've updated it now.
sundersc
force-pushed
the
duplicate-resolver-override
branch
from
9051d44
to
bffe39d
Compare
Description of changes
Avoid creating a duplicate function when overriding a generated resolver.
Issue #, if available
Fixes #9650
Description of how you validated changes
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.