fix(amplify-codegen-appsync-model-plugin): types #5493
Conversation
Fix types regarding nullable lists
| export declare class SimpleNonModelType { | ||
| readonly id: string; | ||
| readonly names?: string[]; | ||
| readonly names?: (string | null)[]; |
There was a problem hiding this comment.
I don't know if there's a unit test to make sure that:
foo: [Bar!]Is generated correctly?
As in that case we would want the following generated since the elements themselves should be non-null, just the array nullable:
readyonly foo?: Bar[];There was a problem hiding this comment.
@danrivett, I added a new Bar type to the tests and included it in the SimpleModel to cover this case.
Codecov Report
@@ Coverage Diff @@
## master #5493 +/- ##
==========================================
+ Coverage 59.08% 59.16% +0.08%
==========================================
Files 402 402
Lines 18043 18072 +29
Branches 3388 3395 +7
==========================================
+ Hits 10661 10693 +32
+ Misses 6743 6741 -2
+ Partials 639 638 -1
Continue to review full report at Codecov.
|
Added test for nullable array having non-null types
| protected getListType(typeStr: string, field: CodeGenField): string { | ||
| return `${typeStr}[]`; | ||
| let type: string = typeStr; | ||
| if (field.isList && field.isNullable) { |
There was a problem hiding this comment.
I'm not familiar with the codegen code so treat accordingly, but would:
| if (field.isList && field.isNullable) { | |
| if (field.isNullable) { |
Be sufficient here?
Do we need to check the isList type since this function is always returning an array type regardless (line 223) and so it seems this if-block only cares if the list element type is nullable or not.
There was a problem hiding this comment.
Yeah I imagine just isNullable is sufficient, I was originally placing this code elsewhere which actually needed it until I found getListType. I am also learning the library haha
Removed redundant isList check
danrivett
left a comment
danrivett
left a comment
There was a problem hiding this comment.
LGTM (as a layperson here at least).
|
@yuth If you can review when you're available, since you reviewed the previous related PRs, it would be much appreciated. Thanks. |
| let ownerFieldName = getOwnerFieldName(rule); | ||
| if (ownerFieldName !== undefined && !hasOwnerField(modelObj.fields, ownerFieldName)) { | ||
| modelDeclarations.addProperty(ownerFieldName, "String", undefined, 'DEFAULT', { | ||
| modelDeclarations.addProperty(ownerFieldName, 'String', undefined, 'DEFAULT', { |
There was a problem hiding this comment.
I think this is where the merging conflict is. I removed this part in my previous merged PR.
There was a problem hiding this comment.
@AaronZyLee , I did not notice the merge conflict... My editor must've refactored to single quotes. I am pushing the merge that resolves the conflict now.
|
@yuth pinging again as this fixes a regression introduced in version 4.29.4 and until it's merged I cannot upgrade from 4.29.3. |
…r list (aws-amplify#5493) Fix types regarding nullable lists Added test for nullable array having non-null types Removed redundant isList check Co-authored-by: Sean Greenawalt <sgreenawalt@dasherlawless.com>
|
This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs. Looking for a help forum? We recommend joining the Amplify Community Discord server |
4 participants
Fix types regarding nullable lists
Issue #, if available: Mentioned in PR #5327
Description of changes:
Fixes an issue with typing with newly added nullable array support for JavaScript and Typescript. Thank you @danrivett for discovering it.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.