Add the outward-bound part of the Cordova redirect flow to auth@exp #4413

sam-gc · 2021-02-04T00:50:22Z

This change handles the generation of the redirect URL including lots of platform-specific handling. This change allows signInWithRedirect() to successfully redirect to the oauth handler / IdP page. The event is not yet handled on return, however.

changeset-bot · 2021-02-04T00:50:24Z

⚠️ No Changeset found

Latest commit: 1669390

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

sam-gc · 2021-02-04T00:51:33Z

packages-exp/auth-exp/src/core/util/handler.ts

+  tid?: string;
+} & { [key: string]: string | undefined };
+
+export function _getRedirectUrl(


This is moved from platform_browser/popup_redirect. The only change is that it now has an "additionalParams" parameter

google-oss-bot · 2021-02-04T01:12:18Z

Size Analysis Report

Affected Products

Diffs between base commit (0eeefe2) and head commit (a1e294c) are too large (169,339 characters) to display.

Please check below links to see details from the original test log.

Test Logs

google-oss-bot · 2021-02-04T01:12:36Z

Binary Size Report

Affected SDKs

@firebase/database-exp

Type Base (0eeefe2) Head (a1e294c) Diff

browser 277 kB 277 kB -59 B (-0.0%)

esm2017 241 kB 241 kB -59 B (-0.0%)

main 279 kB 279 kB -158 B (-0.1%)

module 277 kB 277 kB -59 B (-0.0%)

`@firebase/firestore`

Type	Base (`0eeefe2`)	Head (`a1e294c`)	Diff
browser	259 kB	259 kB	+90 B (+0.0%)
esm2017	201 kB	202 kB	+129 B (+0.1%)
main	530 kB	531 kB	+319 B (+0.1%)
module	259 kB	259 kB	+90 B (+0.0%)
react-native	201 kB	202 kB	+129 B (+0.1%)

`@firebase/firestore-exp`

Type	Base (`0eeefe2`)	Head (`a1e294c`)	Diff
browser	199 kB	199 kB	+138 B (+0.1%)
main	505 kB	505 kB	+319 B (+0.1%)
module	199 kB	199 kB	+138 B (+0.1%)
react-native	199 kB	200 kB	+138 B (+0.1%)

@firebase/firestore-lite

Type Base (0eeefe2) Head (a1e294c) Diff

browser 64.7 kB 64.8 kB +24 B (+0.0%)

module 64.7 kB 64.8 kB +24 B (+0.0%)

react-native 65.0 kB 65.0 kB +24 B (+0.0%)

`@firebase/firestore/bundle`

Type	Base (`0eeefe2`)	Head (`a1e294c`)	Diff
browser	266 kB	266 kB	+84 B (+0.0%)
esm2017	155 kB	155 kB	+142 B (+0.1%)
main	526 kB	527 kB	+319 B (+0.1%)
module	266 kB	266 kB	+84 B (+0.0%)
react-native	155 kB	155 kB	+142 B (+0.1%)

`@firebase/firestore/memory`

Type	Base (`0eeefe2`)	Head (`a1e294c`)	Diff
browser	196 kB	196 kB	+110 B (+0.1%)
esm2017	152 kB	152 kB	+134 B (+0.1%)
main	324 kB	324 kB	+323 B (+0.1%)
module	196 kB	196 kB	+110 B (+0.1%)
react-native	152 kB	152 kB	+134 B (+0.1%)

`@firebase/firestore/memory-bundle`

Type	Base (`0eeefe2`)	Head (`a1e294c`)	Diff
browser	204 kB	205 kB	+104 B (+0.1%)
esm2017	155 kB	155 kB	+142 B (+0.1%)
main	321 kB	321 kB	+323 B (+0.1%)
module	204 kB	205 kB	+104 B (+0.1%)
react-native	155 kB	155 kB	+142 B (+0.1%)

`firebase`

Click to show 15 binary size changes.

Type	Base (`0eeefe2`)	Head (`a1e294c`)	Diff
firebase-analytics.js	35.7 kB	35.6 kB	-7 B (-0.0%)
firebase-app.js	20.1 kB	20.1 kB	-4 B (-0.0%)
firebase-auth.js	177 kB	177 kB	-162 B (-0.1%)
firebase-database.js	195 kB	195 kB	-5 B (-0.0%)
firebase-firestore.js	304 kB	304 kB	-65 B (-0.0%)
firebase-firestore.memory.js	243 kB	243 kB	-12 B (-0.0%)
firebase-functions.js	9.99 kB	9.99 kB	-4 B (-0.0%)
firebase-installations.js	19.0 kB	19.0 kB	-17 B (-0.1%)
firebase-messaging.js	40.8 kB	40.7 kB	-111 B (-0.3%)
firebase-performance-standalone.es2017.js	71.8 kB	71.9 kB	+12 B (+0.0%)
firebase-performance-standalone.js	48.3 kB	48.2 kB	-22 B (-0.0%)
firebase-performance.js	38.5 kB	38.4 kB	-25 B (-0.1%)
firebase-remote-config.js	36.8 kB	36.8 kB	-24 B (-0.1%)
firebase-storage.js	39.8 kB	39.8 kB	+9 B (+0.0%)
firebase.js	856 kB	856 kB	-422 B (-0.0%)

Test Logs

Base (0eeefe29): https://github.com/firebase/firebase-js-sdk/actions/runs/535426062
Head (a1e294ca): https://github.com/firebase/firebase-js-sdk/actions/runs/549537151

yuchenshi · 2021-02-04T18:17:00Z

packages-exp/auth-exp/karma.conf.js

@@ -35,6 +35,8 @@ function getTestFiles(argv) {
    return ['src/**/*.test.ts', 'test/helpers/**/*.test.ts'];
  } else if (argv.integration) {
    return ['test/integration/**/*.test.ts'];
+  } else if (argv.cordova) {
+    return ['src/platform_cordova/**/*.test.ts'];


Question: shall we add the other tests too?

Which other tests?

packages-exp/auth-exp/src/core/util/handler.ts

yuchenshi · 2021-02-04T18:20:11Z

packages-exp/auth-exp/src/model/popup_redirect.ts

@@ -86,7 +86,7 @@ export interface PopupRedirectResolver extends externs.PopupRedirectResolver {
    provider: externs.AuthProvider,
    authType: AuthEventType,
    eventId?: string
-  ): Promise<never>;
+  ): Promise<unknown>;


Why this change?

In Cordova environments, the redirect does not necessarily end the activity, so a call to signInWithRedirect() can actually resolve

packages-exp/auth-exp/src/platform_cordova/popup_redirect/popup_redirect.test.ts

packages-exp/auth-exp/src/core/util/handler.ts

packages-exp/auth-exp/src/platform_cordova/popup_redirect/utils.test.ts

yuchenshi · 2021-02-04T18:37:20Z

packages-exp/auth-exp/src/platform_cordova/popup_redirect/utils.test.ts

+  });
+
+  describe('_performRedirect', () => {
+    let isBrowsertabAvailable: boolean;


Consider following setUA and remove this variable. Each test below explicitly sets isBrowsertabAvailable anyway.

Only one of these has it set to true, the rest to false

yuchenshi · 2021-02-08T17:23:43Z

packages-exp/auth-exp/src/platform_cordova/popup_redirect/utils.ts

+ * protocol).
+ */
+async function computeSha256(sessionId: string): Promise<string> {
+  const encoder = new TextEncoder();


What do we do for IE11? https://caniuse.com/textencoder says no support

I didn't realize Cordova supported IE11 :(

Added a function to fallback to a simple encoding function

packages-exp/auth-exp/src/platform_cordova/popup_redirect/utils.ts

sam-gc added 4 commits February 3, 2021 14:44

Further Cordova redirect support

6b60ba1

Fix

b53e7f1

Add tests for new cordova stuff

b7d8169

Formatting

c767f09

sam-gc requested review from avolkovi and yuchenshi February 4, 2021 00:50

sam-gc assigned yuchenshi Feb 4, 2021

sam-gc commented Feb 4, 2021

View reviewed changes

Formatting

3a51fc5

yuchenshi approved these changes Feb 8, 2021

View reviewed changes

sam-gc added 2 commits February 8, 2021 14:43

PR feedback

0d1f785

Formatting

1669390

sam-gc merged commit 96bcb98 into master Feb 8, 2021

sam-gc deleted the samgho/cdv-pr branch February 8, 2021 23:16

firebase locked and limited conversation to collaborators Mar 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the outward-bound part of the Cordova redirect flow to auth@exp #4413

Add the outward-bound part of the Cordova redirect flow to auth@exp #4413

sam-gc commented Feb 4, 2021

changeset-bot bot commented Feb 4, 2021 •

edited

Loading

sam-gc Feb 4, 2021

google-oss-bot commented Feb 4, 2021 •

edited

Loading

Affected Products

Test Logs

google-oss-bot commented Feb 4, 2021 •

edited

Loading

`@firebase/database-exp`

`@firebase/firestore`

`@firebase/firestore-exp`

`@firebase/firestore-lite`

`@firebase/firestore/bundle`

`@firebase/firestore/memory`

`@firebase/firestore/memory-bundle`

`firebase`

yuchenshi Feb 4, 2021

sam-gc Feb 8, 2021

yuchenshi Feb 4, 2021

sam-gc Feb 8, 2021

yuchenshi Feb 4, 2021

sam-gc Feb 8, 2021

yuchenshi Feb 8, 2021

sam-gc Feb 8, 2021

sam-gc Feb 8, 2021

Add the outward-bound part of the Cordova redirect flow to auth@exp #4413

Add the outward-bound part of the Cordova redirect flow to auth@exp #4413

Conversation

sam-gc commented Feb 4, 2021

changeset-bot bot commented Feb 4, 2021 • edited Loading

⚠️ No Changeset found

Choose a reason for hiding this comment

google-oss-bot commented Feb 4, 2021 • edited Loading

Size Analysis Report

Affected Products

Test Logs

google-oss-bot commented Feb 4, 2021 • edited Loading

Binary Size Report

Affected SDKs

@firebase/database-exp

@firebase/firestore

@firebase/firestore-exp

@firebase/firestore-lite

@firebase/firestore/bundle

@firebase/firestore/memory

@firebase/firestore/memory-bundle

firebase

Test Logs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Feb 4, 2021 •

edited

Loading

google-oss-bot commented Feb 4, 2021 •

edited

Loading

google-oss-bot commented Feb 4, 2021 •

edited

Loading

`@firebase/database-exp`

`@firebase/firestore`

`@firebase/firestore-exp`

`@firebase/firestore-lite`

`@firebase/firestore/bundle`

`@firebase/firestore/memory`

`@firebase/firestore/memory-bundle`

`firebase`