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
feat(core): add AmplifyOutputs types based on the CLI Gen2 client config schema #4859
Conversation
part 'amplify_outputs.g.dart'; | ||
|
||
/// {@template amplify_core.amplify_outputs} | ||
/// The amplify outputs generated by Amplify CLI Gen2 for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think "CLI" isn't being used when referring to gen 2. "Gen 2" should have a space.
/// The amplify outputs generated by Amplify CLI Gen2 for | |
/// The amplify outputs generated by Amplify Gen 2 for |
/// {@macro amplify_core.amplify_outputs.storage_outputs} | ||
final StorageOutputs? storage; | ||
|
||
/// The custom outputs generated by CLI Gen2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// The custom outputs generated by CLI Gen2. | |
/// The custom outputs generated by Amplify Gen 2. |
packages/amplify_core/lib/src/config/amplify_outputs/amplify_outputs.dart
Show resolved
Hide resolved
packages/amplify_core/lib/src/config/amplify_outputs/notifications/notifications_outputs.dart
Show resolved
Hide resolved
/// The Amazon Pinpoint outputs used by Analytics category plugin | ||
/// to communicate with backend services. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit/suggestion: Trim these a bit and remove the mention of what category it is for. The customer will know what category it is used for since it is nested under the category.
Q: This class is only used for Analytics, but couldn't it potentially be re-used for Push Notification, or some other category that uses pinpoint?
/// The Amazon Pinpoint outputs used by Analytics category plugin | |
/// to communicate with backend services. | |
/// The Amplify Gen 2 output for Amazon Pinpoint. |
this.standardRequiredAttributes, | ||
this.usernameAttributes, | ||
this.userVerificationTypes, | ||
this.unauthenticatedIdentitiesEnabled = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: It looks like this is new. Gen 1 config does not have this AFAIK. Are we supposed to make any changes in amplify auth to consume this? For example, should amplify auth throw when fetchAuthSession is called for an unauthenticated user if this is false? Is there a task tracking that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, the Gen 1 config does not have this field. Guest access is allowed if it's enabled on the backend. Libraries initiate the request, and upon success, they get guest credentials otherwise an error is returned. Android and iOS also ignore this value from Gen 2 config meaning no changes are needed within Amplify Auth to use this value.
/// The MFA method of Amazon Cognito User Pool for | ||
/// sending the authorization code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: TOTP codes are not "sent" so I would remove that portion.
/// The MFA method of Amazon Cognito User Pool for | |
/// sending the authorization code. | |
/// The MFA method of Amazon Cognito User Pool. |
final bool? requireNumbers; | ||
|
||
/// Requires lower case. | ||
final bool? requireLowercase; | ||
|
||
/// Requires upper case. | ||
final bool? requireUppercase; | ||
|
||
/// Requires symbols. | ||
final bool? requireSymbols; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Should all the bool values default to false? The auth/authenticator libraries will have to handle the null case as is. IMO the default values belong here not in the auth/authenticator categories.
/// {@template amplify_core.amplify_outputs.response_type} | ||
/// The response type for identity providers set on Amazon Cognito. | ||
/// {@endtemplate} | ||
enum ResponseType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: OAuthResponseType
might be a better name.
270f8ba
to
21a0c89
Compare
21a0c89
to
647cbac
Compare
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.