Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(@aws-amplify/auth): define validationData type as object literal #8045

Merged
merged 18 commits into from
Apr 13, 2021

Conversation

nickarocho
Copy link
Contributor

Description of changes

  • fixes TypeScript error when passing validationData
  • fixes ts-jest linting errors when running yarn test
  • adds a unit test for passing validationData to the Auth.signUp method

In JavaScript, users can simply pass validationData as an object literal with 'key': 'value' pairs, but in TypeScript we get the following error:

Screen Shot 2021-04-05 at 12 42 27 PM

This is because the SignUpParams interface was defining validationData as an array of objects (via ICognitoUserAttributeData[]).

This fixes the problem for TypeScript users, but we will still need to update the documentation around any reference of validationData to make sure users are just passing object literals rather than an array of objects.

Issue #, if available

#7774

Description of how you validated changes

I made a sample app from scratch, imported Amplify (@latest), imported Auth (@latest), manually called Auth.signUp, and passed different sets of validationData.

We do see "SerializationException" errors from the server response when passing the values as integers (i.e. "NUMBER_VALUE can not be converted to a String"), arrays (i.e. "Start of list found where not expected", and objects (i.e. "Start of structure or map found where not expected."), so maybe it would be better to define the type as:
validationData?: { [key: string]: string };.

I also wrote a new test as well to validate the type passed as a param matches what we expect from TypeScript - thanks to @iartemiev's recent updates, these types are now being explicitly checked in the auth testing suites.

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

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

@nickarocho nickarocho self-assigned this Apr 7, 2021
@nickarocho nickarocho requested a review from elorzafe as a code owner April 7, 2021 20:09
@nickarocho nickarocho added Auth Related to Auth components/category TypeScript Related to TypeScript issues labels Apr 7, 2021
@codecov
Copy link

codecov bot commented Apr 7, 2021

Codecov Report

Merging #8045 (f252195) into main (c4ccc17) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8045      +/-   ##
==========================================
+ Coverage   75.42%   75.44%   +0.02%     
==========================================
  Files         215      215              
  Lines       13548    13548              
  Branches     2673     2673              
==========================================
+ Hits        10218    10221       +3     
+ Misses       3127     3124       -3     
  Partials      203      203              
Impacted Files Coverage Δ
packages/auth/src/types/Auth.ts 100.00% <ø> (ø)
packages/auth/src/Auth.ts 88.52% <0.00%> (+0.34%) ⬆️

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 c4ccc17...f252195. Read the comment docs.

Copy link
Contributor

@amhinson amhinson left a comment

Choose a reason for hiding this comment

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

Nice! LGTM 👍

Copy link
Member

@iartemiev iartemiev left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@nickarocho nickarocho merged commit 6d0b67c into aws-amplify:main Apr 13, 2021
@nickarocho nickarocho deleted the auth-validationData-type-fix branch April 13, 2021 17:49
@WIStudent
Copy link

Looking at https://github.com/aws-amplify/amplify-js/blob/main/packages/auth/src/Auth.ts#L326 I think the intended usage of validationData is now with an object containing string values (CognitoUserAttribute type expects a string as Value) so I think validationData's type should be validationData?: { [key: string]: string };

The current type validationData?: { [key: string]: any } does not throw a typescript error if I try to pass something like

validationData: [
  new CognitoUserAttribute({Name: 'foo', Value: 'bar'})
]

but the implementation will throw a SerializationException as runtime error. I would rather prefer a typescript error at compile time than a runtime error when a user is using my project.

@github-actions
Copy link

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 or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Auth Related to Auth components/category TypeScript Related to TypeScript issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants