-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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): listen for pageshow
event listener
#13278
Conversation
packages/auth/src/providers/cognito/utils/oauth/cancelOAuthFlow.ts
Outdated
Show resolved
Hide resolved
packages/auth/src/providers/cognito/utils/oauth/cancelOAuthFlow.navite.ts
Outdated
Show resolved
Hide resolved
import { OAuthStore } from '../types'; | ||
|
||
import { createOAuthError } from './createOAuthError'; | ||
import { handleFailure, } from './index'; |
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.
window.removeEventListener('pageshow', handleCancelOauthFlow); | ||
} | ||
} | ||
window.addEventListener('pageshow', handleCancelOauthFlow); |
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 pageshow event is sent to a Window when the browser displays the window's document due to navigation.
This includes:
- Initially loading the page
- Navigating to the page from another page in the same window or tab
- Restoring a frozen page on mobile OSes
- Returning to the page using the browser's forward or back buttons
This seemed too broad to me than what we are looking for (the last case).
Any possibility to use popstate
event which is more closely tight to the navigation?
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.
I see the popstate
will be fired only if the history state has changed, also it doesn't provide a way to identify whether the page was initially being loaded or not, whereas the pageshow
event does. Although pageshow
has more triggers, this event will most likely be triggered when the user has clicked the back button.
async function handleCancelOauthFlow(event: PageTransitionEvent) { | ||
const isOAuthInFlight = await store.loadOAuthInFlight(); | ||
if (event.persisted && isOAuthInFlight) { | ||
await handleFailure(createOAuthError('User cancelled OAuth flow.')); |
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.
Not sure that a cancellation event (via end user navigate back) should be handled as failure and correlated to an error. Providing a Hub
event makes sense, but imo should denote a cancellation (e.g. "signInWithRedirect_cancel"
) not an error since there is no failure, an end user canceled the flow
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.
Yeah this is a good point. However we are currently surfacing the same signInWithRedirect_failure
hub event when bfcache is not enabled. I just wanted to keep consistency. Also it will be a change in DX. If a customer that was working locally with no bfcache and was already listening to the failure event on cancellation, then they would need to also consider this new event.
Description of changes
Currently when bfcache is enabled and the user clicks the back button, amplify is not able to detect the cancellation because
Amplify.configure
is never called. This change listens for thepageshow
window event and clears any inflight oauth keysIssue #, if available
#13274
Description of how you validated changes
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.