Skip to content

Conversation

@seang96
Copy link
Contributor

@seang96 seang96 commented Sep 14, 2020

Issue #, if available: #5139

Description of changes:
I added a check to getTypeInfo to check if the current node is NonNullType and its' child node is ListType to set isListNullable, new attribute in TypeInfo, to to true.
The metadata-visitor then sets isArrayRequired to true whe isListNullable is not allowed.
To add support in DataStore, this PR should do: aws-amplify/amplify-js#6784

This PR should support the table below:

declaration null [] [null] [{ foo: 'BAR' }]
[Type!]! no yes no yes
[Type]! no yes yes yes
[Type!] yes yes no yes
[Type] yes yes yes yes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Sep 14, 2020

Codecov Report

Merging #5327 into master will increase coverage by 0.00%.
The diff coverage is 75.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5327   +/-   ##
=======================================
  Coverage   59.86%   59.86%           
=======================================
  Files         391      391           
  Lines       17469    17473    +4     
  Branches     3470     3472    +2     
=======================================
+ Hits        10458    10461    +3     
- Misses       6388     6389    +1     
  Partials      623      623           
Impacted Files Coverage Δ
...psync-model-plugin/src/visitors/appsync-visitor.ts 95.67% <ø> (ø)
...ugin/src/visitors/appsync-json-metadata-visitor.ts 93.75% <50.00%> (-1.42%) ⬇️
...en-appsync-model-plugin/src/utils/get-type-info.ts 90.00% <100.00%> (+2.50%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d22daaa...45233fc. Read the comment docs.

Rename isArrayRequired to isArrayNullable
};

if (field.isListNullable !== undefined) {
fieldMeta.isArrayNullable = !field.isListNullable;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the rename, this should be changed to

Suggested change
fieldMeta.isArrayNullable = !field.isListNullable;
fieldMeta.isArrayNullable = field.isListNullable;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuth, Ah I did not think about this. I added a commit to fix this.

isArrayNullable value should be flipped due to name change
@yuth yuth merged commit c88bee1 into aws-amplify:master Sep 25, 2020
@seang96
Copy link
Contributor Author

seang96 commented Sep 30, 2020

@yuth, I discovered an issue due to the rename to isArrayNullable having the opposite meaning means in getTypeInfo it should only return if isListNullable is true instead of if it is false. I am submitting a PR that fixes this.

@danrivett
Copy link

danrivett commented Oct 5, 2020

@yuth just an FYI that this looks like the cause that broke our front-end when we upgraded from @aws-amplify/cli package version from 4.29.3 to 4.29.4 since types that are defined as:

foo: [Bar!]

(e.g. the array is optional) stopped being generated as:

readonly foo?: Bar[];

but instead got generated as:

readonly foo: Bar[];

I haven't yet read this ticket in detail to know if that was an intentional change or not, but if it was, I would suggest it wasn't a patch-level change since it caused our RN app using those types to start generating TypeScript errors.

I think overall the change is likely a good one as we were having to work around DataStore not being able to handle empty arrays and so instead we were deliberately setting the values to null as that was working for us. So the change looks like a good change, it was more the surprise of it being released as a patch release and not seeing anything in the changelog under 4.29.4 mentioning this change.

Update: Perhaps #5450 fixes this issue we ran into but hasn't yet been released, and so isn't included in 4.29.4? If it is included in that release, then I'll need to understand this more as I haven't yet read through both of the PRs as kind of caught us off guard on a Monday morning.

@seang96
Copy link
Contributor Author

seang96 commented Oct 5, 2020

@danrivett I looked into this, #5450 is in the latest version. The schema is generating correctly with isArrayNullable true and isRequired as true, but the types are not correct. I'll look into this and submit a PR for types. Sorry for the bug!

@danrivett
Copy link

@danrivett I looked into this, #5450 is in the latest version. The schema is generating correctly with isArrayNullable true and isRequired as true, but the types are not correct. I'll look into this and submit a PR for types. Sorry for the bug!

Thanks for the update @seang96, no problem.

Thank you for looking into this. For now we've reverted to 4.29.3 so unblocked.

r0zar pushed a commit to r0zar/amplify-cli that referenced this pull request Nov 19, 2020
@github-actions
Copy link

github-actions bot commented Oct 6, 2021

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 *-help channels for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants