From fdadf71cb4f5bbaed084e984724ffe9c2279fe44 Mon Sep 17 00:00:00 2001 From: Sam Horlbeck Olsen Date: Mon, 22 Mar 2021 15:21:16 -0700 Subject: [PATCH] [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 --- packages-exp/auth-compat-exp/index.node.ts | 1 + packages-exp/auth-compat-exp/index.ts | 6 +- .../auth-compat-exp/rollup.config.shared.js | 2 +- packages-exp/auth-compat-exp/src/auth.test.ts | 36 ++++++++---- packages-exp/auth-compat-exp/src/auth.ts | 57 +++++++++++-------- .../auth-compat-exp/src/persistence.ts | 9 +-- .../test/integration/flows/anonymous.test.ts | 2 - .../test/integration/flows/custom.test.ts | 1 - 8 files changed, 65 insertions(+), 49 deletions(-) diff --git a/packages-exp/auth-compat-exp/index.node.ts b/packages-exp/auth-compat-exp/index.node.ts index a11ac09c2be..c780f3bd8aa 100644 --- a/packages-exp/auth-compat-exp/index.node.ts +++ b/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, diff --git a/packages-exp/auth-compat-exp/index.ts b/packages-exp/auth-compat-exp/index.ts index 626fb15726e..563f68ad422 100644 --- a/packages-exp/auth-compat-exp/index.ts +++ b/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); diff --git a/packages-exp/auth-compat-exp/rollup.config.shared.js b/packages-exp/auth-compat-exp/rollup.config.shared.js index e4cce57e7f1..aa82c712916 100644 --- a/packages-exp/auth-compat-exp/rollup.config.shared.js +++ b/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 } }, /** diff --git a/packages-exp/auth-compat-exp/src/auth.test.ts b/packages-exp/auth-compat-exp/src/auth.test.ts index 9b79cd45a42..73888aec38e 100644 --- a/packages-exp/auth-compat-exp/src/auth.test.ts +++ b/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>; + 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 + ] + } + }); } }); }); diff --git a/packages-exp/auth-compat-exp/src/auth.ts b/packages-exp/auth-compat-exp/src/auth.ts index 0d7b3dd7071..422cf7c6845 100644 --- a/packages-exp/auth-compat-exp/src/auth.ts +++ b/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, _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._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 { diff --git a/packages-exp/auth-compat-exp/src/persistence.ts b/packages-exp/auth-compat-exp/src/persistence.ts index 7ccd855c493..93ba4fe6c8f 100644 --- a/packages-exp/auth-compat-exp/src/persistence.ts +++ b/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) { diff --git a/packages-exp/auth-compat-exp/test/integration/flows/anonymous.test.ts b/packages-exp/auth-compat-exp/test/integration/flows/anonymous.test.ts index d953bd9f402..ddb89bf3d71 100644 --- a/packages-exp/auth-compat-exp/test/integration/flows/anonymous.test.ts +++ b/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, diff --git a/packages-exp/auth-compat-exp/test/integration/flows/custom.test.ts b/packages-exp/auth-compat-exp/test/integration/flows/custom.test.ts index b0b69893fb3..fdeb1505905 100644 --- a/packages-exp/auth-compat-exp/test/integration/flows/custom.test.ts +++ b/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 {