[Auth compat] Fix the way we initialize compat auth. Also fix some broken integration tests (#4653)

* Fix auth compat initialization

* Fix compat lint

* Fix broken node tests

* Formatting

* Remove dead code

Author: sam-gc

packages-exp/auth-compat-exp/index.node.ts

@@ -24,6 +24,7 @@
 export * from './index';
 import { FetchProvider } from '@firebase/auth-exp/internal';
 import * as fetchImpl from 'node-fetch';
+import './index';
 
 FetchProvider.initialize(
   (fetchImpl.default as unknown) as typeof fetch,

packages-exp/auth-compat-exp/index.ts

@@ -98,8 +98,8 @@ function registerAuthCompat(instance: _FirebaseNamespace): void {
       container => {
         // getImmediate for FirebaseApp will always succeed
         const app = container.getProvider('app-compat').getImmediate();
-        const auth = container.getProvider('auth-exp').getImmediate();
-        return new Auth(app, auth as impl.AuthImpl);
+        const authProvider = container.getProvider('auth-exp');
+        return new Auth(app, authProvider);
       },
       ComponentType.PUBLIC
     )
@@ -136,7 +136,7 @@ function registerAuthCompat(instance: _FirebaseNamespace): void {
       .setMultipleInstances(false)
   );
 
-  instance.registerVersion('auth', version);
+  instance.registerVersion('auth-compat', version);
 }
 
 registerAuthCompat(firebase as _FirebaseNamespace);

packages-exp/auth-compat-exp/rollup.config.shared.js

@@ -65,7 +65,7 @@ export function getEs5Builds(additionalTypescriptPlugins = {}) {
       plugins: es5BuildPlugins,
       external: id => deps.some(dep => id === dep || id.startsWith(`${dep}/`)),
       treeshake: {
-        moduleSideEffects: false
+        moduleSideEffects: true
       }
     },
     /**

packages-exp/auth-compat-exp/src/auth.test.ts

@@ -17,6 +17,7 @@
 
 import { FirebaseApp } from '@firebase/app-compat';
 import * as exp from '@firebase/auth-exp/internal';
+import { Provider } from '@firebase/component';
 import { expect, use } from 'chai';
 import * as sinon from 'sinon';
 import * as sinonChai from 'sinon-chai';
@@ -31,12 +32,16 @@ describe('auth compat', () => {
   context('redirect persistence key storage', () => {
     let underlyingAuth: exp.AuthImpl;
     let app: FirebaseApp;
+    let providerStub: sinon.SinonStubbedInstance<Provider<'auth-exp'>>;
+
     beforeEach(() => {
       app = { options: { apiKey: 'api-key' } } as FirebaseApp;
       underlyingAuth = new exp.AuthImpl(app, {
         apiKey: 'api-key'
       } as exp.Config);
       sinon.stub(underlyingAuth, '_initializeWithPersistence');
+
+      providerStub = sinon.createStubInstance(Provider);
     });
 
     afterEach(() => {
@@ -56,7 +61,12 @@ describe('auth compat', () => {
           ),
           '_openRedirect'
         );
-        const authCompat = new Auth(app, underlyingAuth);
+        providerStub.isInitialized.returns(true);
+        providerStub.getImmediate.returns(underlyingAuth);
+        const authCompat = new Auth(
+          app,
+          (providerStub as unknown) as Provider<'auth-exp'>
+        );
         // eslint-disable-next-line @typescript-eslint/no-floating-promises
         await authCompat.signInWithRedirect(new exp.GoogleAuthProvider());
         expect(
@@ -71,18 +81,20 @@ describe('auth compat', () => {
           'firebase:persistence:api-key:undefined',
           'none'
         );
-        new Auth(app, underlyingAuth);
+        providerStub.isInitialized.returns(false);
+        providerStub.initialize.returns(underlyingAuth);
+        new Auth(app, (providerStub as unknown) as Provider<'auth-exp'>);
         // eslint-disable-next-line @typescript-eslint/no-floating-promises
-        expect(
-          underlyingAuth._initializeWithPersistence
-        ).to.have.been.calledWith(
-          [
-            exp._getInstance(exp.inMemoryPersistence),
-            exp._getInstance(exp.indexedDBLocalPersistence),
-            exp._getInstance(exp.browserLocalPersistence)
-          ],
-          CompatPopupRedirectResolver
-        );
+        expect(providerStub.initialize).to.have.been.calledWith({
+          options: {
+            popupRedirectResolver: CompatPopupRedirectResolver,
+            persistence: [
+              exp.inMemoryPersistence,
+              exp.indexedDBLocalPersistence,
+              exp.browserLocalPersistence
+            ]
+          }
+        });
       }
     });
   });

packages-exp/auth-compat-exp/src/auth.ts

@@ -18,6 +18,7 @@
 import { FirebaseApp, _FirebaseService } from '@firebase/app-compat';
 import * as exp from '@firebase/auth-exp/internal';
 import * as compat from '@firebase/auth-types';
+import { Provider } from '@firebase/component';
 import { ErrorFn, Observer, Unsubscribe } from '@firebase/util';
 
 import {
@@ -39,47 +40,55 @@ const _assert: typeof exp._assert = exp._assert;
 
 export class Auth
   implements compat.FirebaseAuth, Wrapper<exp.Auth>, _FirebaseService {
-  // private readonly auth: impl.AuthImpl;
+  private readonly auth: exp.AuthImpl;
 
-  constructor(readonly app: FirebaseApp, private readonly auth: exp.AuthImpl) {
-    const { apiKey } = app.options;
-    if (this.auth._deleted) {
+  constructor(readonly app: FirebaseApp, provider: Provider<'auth-exp'>) {
+    if (provider.isInitialized()) {
+      this.auth = provider.getImmediate() as exp.AuthImpl;
       return;
     }
 
-    // Note this is slightly different behavior: in this case, the stored
-    // persistence is checked *first* rather than last. This is because we want
-    // to prefer stored persistence type in the hierarchy.
-    const persistences = _getPersistencesFromRedirect(this.auth);
+    const { apiKey } = app.options;
+    // TODO: platform needs to be determined using heuristics
+    _assert(apiKey, exp.AuthErrorCode.INVALID_API_KEY, {
+      appName: app.name
+    });
+
+    let persistences: exp.Persistence[] = [exp.inMemoryPersistence];
 
-    for (const persistence of [
-      exp.indexedDBLocalPersistence,
-      exp.browserLocalPersistence
-    ]) {
-      if (!persistences.includes(persistence)) {
-        persistences.push(persistence);
+    // Only deal with persistences in web environments
+    if (typeof window !== 'undefined') {
+      // Note this is slightly different behavior: in this case, the stored
+      // persistence is checked *first* rather than last. This is because we want
+      // to prefer stored persistence type in the hierarchy.
+      persistences = _getPersistencesFromRedirect(apiKey, app.name);
+
+      for (const persistence of [
+        exp.indexedDBLocalPersistence,
+        exp.browserLocalPersistence
+      ]) {
+        if (!persistences.includes(persistence)) {
+          persistences.push(persistence);
+        }
       }
     }
 
-    const hierarchy = persistences.map<exp.PersistenceInternal>(
-      exp._getInstance
-    );
-
     // TODO: platform needs to be determined using heuristics
     _assert(apiKey, exp.AuthErrorCode.INVALID_API_KEY, {
       appName: app.name
     });
 
-    this.auth._updateErrorMap(exp.debugErrorMap);
-
     // Only use a popup/redirect resolver in browser environments
     const resolver =
       typeof window !== 'undefined' ? CompatPopupRedirectResolver : undefined;
+    this.auth = provider.initialize({
+      options: {
+        persistence: persistences,
+        popupRedirectResolver: resolver
+      }
+    }) as exp.AuthImpl;
 
-    // This promise is intended to float; auth initialization happens in the
-    // background, meanwhile the auth object may be used by the app.
-    // eslint-disable-next-line @typescript-eslint/no-floating-promises
-    this.auth._initializeWithPersistence(hierarchy, resolver);
+    this.auth._updateErrorMap(exp.debugErrorMap);
   }
 
   get emulatorConfig(): compat.EmulatorConfig | null {

packages-exp/auth-compat-exp/src/persistence.ts

@@ -97,18 +97,15 @@ export async function _savePersistenceForRedirect(
 }
 
 export function _getPersistencesFromRedirect(
-  auth: exp.AuthInternal
+  apiKey: string,
+  appName: string
 ): exp.Persistence[] {
   const win = getSelfWindow();
   if (!win?.sessionStorage) {
     return [];
   }
 
-  const key = exp._persistenceKeyName(
-    PERSISTENCE_KEY,
-    auth.config.apiKey,
-    auth.name
-  );
+  const key = exp._persistenceKeyName(PERSISTENCE_KEY, apiKey, appName);
   const persistence = win.sessionStorage.getItem(key);
 
   switch (persistence) {

packages-exp/auth-compat-exp/test/integration/flows/anonymous.test.ts

@@ -19,8 +19,6 @@ import { expect, use } from 'chai';
 import * as chaiAsPromised from 'chai-as-promised';
 
 import firebase from '@firebase/app-compat';
-// eslint-disable-next-line import/no-extraneous-dependencies
-import '@firebase/auth-compat';
 import { FirebaseError } from '@firebase/util';
 import {
   cleanUpTestInstance,

packages-exp/auth-compat-exp/test/integration/flows/custom.test.ts

@@ -18,7 +18,6 @@
 import { FirebaseError } from '@firebase/util';
 import { expect, use } from 'chai';
 import firebase from '@firebase/app-compat';
-import '@firebase/auth-compat';
 
 import * as chaiAsPromised from 'chai-as-promised';
 import {