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(auth): Cancel sign in #2840

Merged
merged 3 commits into from Apr 12, 2023
Merged

fix(auth): Cancel sign in #2840

merged 3 commits into from Apr 12, 2023

Conversation

dnys1
Copy link
Contributor

@dnys1 dnys1 commented Apr 11, 2023

Properly enqueues sign-in calls and cancels previous attempts when signing in or confirming a sign in.

@dnys1 dnys1 requested a review from a team as a code owner April 11, 2023 18:50
Properly enqueues sign-in calls and cancels previous attempts when
To prevent `dart:io` from being imported in JS code.
@@ -614,28 +604,22 @@ class AmplifyAuthCognitoDart extends AuthPluginInterface
signInStep: AuthSignInStep.done,
),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe extract the switch-case to a function as it is used for signIn and confirmSignIn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@@ -47,7 +47,7 @@ class SignInStateMachine extends AuthStateMachine<SignInEvent, SignInState> {
SignInState get initialState => const SignInState.notStarted();

/// Flow used to sign in.
late final AuthFlowType authFlowType;
Copy link
Contributor

Choose a reason for hiding this comment

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

why not final?

@@ -59,7 +59,7 @@ class SignInStateMachine extends AuthStateMachine<SignInEvent, SignInState> {
}();

/// Parameters to the flow.
late final SignInParameters parameters;
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same state machine can be used for multiple sign ins.

Comment on lines 561 to 563
await _stateMachine.acceptAndComplete<SignInNotStarted>(
const SignInEvent.cancelled(),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

SignInStateMachine.resolve() calls _assertSignedOut() and _reset() before running the SignInEvent.initiate. why do we need to cancel sign in flow if any, isn't it taken care by the SignInStateMachine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm yes this was leftover from earlier iterations. Removed 👍

@dnys1 dnys1 merged commit 3a2eea9 into next Apr 12, 2023
77 of 81 checks passed
@dnys1 dnys1 deleted the fix/auth/cancel-sign-in branch April 12, 2023 20:20
dnys1 pushed a commit that referenced this pull request Apr 12, 2023
### Features
- Publish push notifications plugin

### Fixes
- fix(auth): Cancel sign in ([#2840](#2840))

Updated-Components: amplify_flutter, amplify_flutter_ios, amplify_core, amplify_datastore, amplify_datastore_plugin_interface, amplify_analytics_pinpoint, amplify_analytics_pinpoint_dart, amplify_api, amplify_auth_cognito, amplify_auth_cognito_dart, aws_common, amplify_db_common, amplify_push_notifications, amplify_push_notifications_pinpoint, amplify_storage_s3, amplify_storage_s3_dart, Amplify UI, Secure Storage, Smithy, Worker Bee
dnys1 pushed a commit that referenced this pull request Apr 12, 2023
### Features
- Publish push notifications plugin

### Fixes
- fix(auth): Cancel sign in ([#2840](#2840))

Updated-Components: amplify_flutter, amplify_flutter_ios, amplify_core, amplify_datastore, amplify_datastore_plugin_interface, amplify_analytics_pinpoint, amplify_analytics_pinpoint_dart, amplify_api, amplify_auth_cognito, amplify_auth_cognito_dart, aws_common, amplify_db_common, amplify_push_notifications, amplify_push_notifications_pinpoint, amplify_storage_s3, amplify_storage_s3_dart, Amplify UI, Secure Storage, Smithy, Worker Bee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants