feat(rtn-web-browser): add auth support for chromebook #14523
Conversation
ahmedhamouda78
requested review from
pranavosu,
Amplifiyer and
avi-karthik
as code owners
Co-authored-by: Ahmed Hamouda <[email protected]>
packages/rtn-web-browser/src/apis/__tests__/openAuthSessionAsync.test.ts
Show resolved
Hide resolved
| export async function isChromebook(): Promise<boolean> { | ||
| // expo go | ||
| try { | ||
| const Device = require('expo-device'); |
There was a problem hiding this comment.
I don't understand.
do we expect users to install expo-device optionally on their own?
why not include it as a dependency? and if we cannot include it as a dependency, why is this logic needed at all?
There was a problem hiding this comment.
We don't require expo-device as a dependency. However, customers are free to use it with their Amplify react-native apps, so this line is just to support device detection in case customers are using expo. If customers are not using expo, we fallback to NativeModules on line 36
There was a problem hiding this comment.
got it. thanks for the explanation
| getAppStatePromise(), | ||
| ]), | ||
| Linking.openURL(url), | ||
| ]); |
There was a problem hiding this comment.
this is hard to follow.
this races between, 2 promises
- whenever a "linking" event happens
- when the app leaves inactive state and becomes active.
and then this race is part of an all, which resolves when the race resolved and an url has been opened.
doesn't Linking.openURL automatically mean that getRedirectPromise gets resolved.
as a result the same URL, which was just opened gets returned by this Promise construct
because:
if (redirectUrls.some(url => event.url.startsWith(url))) {
resolve(event.url);
}
as
getRedirectPromisesets up a listener on "Linking"-url change and resolves with that URL when this happensgetAppStatePromisesets up a listener on "going active" and resolves withnull- openURL - navigates
so that.
- navigate.
- getRedirectPromise-emits
- redirectUrl === url
and then this makes another call to openURL with the same URL
There was a problem hiding this comment.
resolved this offline.
the solution uses Linking instead of native navigation if the latter fails to open the URL
| export async function isChromebook(): Promise<boolean> { | ||
| // expo go | ||
| try { | ||
| const Device = require('expo-device'); |
There was a problem hiding this comment.
got it. thanks for the explanation
| getAppStatePromise(), | ||
| ]), | ||
| Linking.openURL(url), | ||
| ]); |
There was a problem hiding this comment.
resolved this offline.
the solution uses Linking instead of native navigation if the latter fails to open the URL
Description of changes
This PR adds authentication support for Chromebook devices in the
rtn-web-browserpackage. The changes include:isChromebook()function to detect Chromebook devices using Expo Device API and native ChromeOS moduleopenAuthSessionChromeOs()function to handle auth sessions on Chromebook usingLinking.openURL()instead of native browser tabsopenAuthSessionAsync()to detect Chromebook devices and route to the appropriate auth session handlerIssue #, if available
#14459
Description of how you validated changes
Checklist
yarn testpassesChecklist for repo maintainers
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.