From 8403a4741e5f3db65812e71dffeabc137ace42c1 Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Wed, 11 Aug 2021 14:07:26 -0700 Subject: [PATCH 1/7] Make initializeAuth() idempotent --- .../auth-exp/src/core/auth/initialize.test.ts | 49 +++++++++++++++++-- .../auth-exp/src/core/auth/initialize.ts | 17 +++++-- 2 files changed, 58 insertions(+), 8 deletions(-) diff --git a/packages-exp/auth-exp/src/core/auth/initialize.test.ts b/packages-exp/auth-exp/src/core/auth/initialize.test.ts index 63a9864b187..e7d3e3d4f93 100644 --- a/packages-exp/auth-exp/src/core/auth/initialize.test.ts +++ b/packages-exp/auth-exp/src/core/auth/initialize.test.ts @@ -31,7 +31,7 @@ import { import { isNode } from '@firebase/util'; import { expect } from 'chai'; -import { inMemoryPersistence } from '../../../internal'; +import { browserLocalPersistence, browserSessionPersistence, inMemoryPersistence } from '../../../internal'; import { AuthInternal } from '../../model/auth'; import { @@ -50,6 +50,7 @@ import { import { ClientPlatform, _getClientVersion } from '../util/version'; import { initializeAuth } from './initialize'; import { registerAuth } from './register'; +import { debugErrorMap, prodErrorMap } from '../errors'; describe('core/auth/initialize', () => { let fakeApp: FirebaseApp; @@ -209,9 +210,49 @@ describe('core/auth/initialize', () => { expect(auth._isInitialized).to.be.false; }); - it('should throw if called more than once', () => { - initializeAuth(fakeApp); - expect(() => initializeAuth(fakeApp)).to.throw(); + it('should not throw if called again with same (no) params', () => { + const auth = initializeAuth(fakeApp); + expect(initializeAuth(fakeApp)).to.equal(auth); + }); + + it('should not throw if called again with same params', () => { + const auth = initializeAuth(fakeApp, { + errorMap: prodErrorMap, + persistence: browserSessionPersistence, + popupRedirectResolver: fakePopupRedirectResolver + }); + expect(initializeAuth(fakeApp, { + errorMap: prodErrorMap, + persistence: browserSessionPersistence, + popupRedirectResolver: fakePopupRedirectResolver + })).to.equal(auth); + }); + + it('should throw if called again with different params (popupRedirectResolver)', () => { + initializeAuth(fakeApp, { + popupRedirectResolver: fakePopupRedirectResolver + }); + expect(() => initializeAuth(fakeApp, { + popupRedirectResolver: undefined + })).to.throw(); + }); + + it('should throw if called again with different params (errorMap)', () => { + initializeAuth(fakeApp, { + errorMap: prodErrorMap + }); + expect(() => initializeAuth(fakeApp, { + errorMap: debugErrorMap + })).to.throw(); + }); + + it('should throw if called again with different params (persistence)', () => { + initializeAuth(fakeApp, { + persistence: [browserLocalPersistence, browserSessionPersistence] + }); + expect(() => initializeAuth(fakeApp, { + persistence: [browserSessionPersistence, browserLocalPersistence] + })).to.throw(); }); }); }); diff --git a/packages-exp/auth-exp/src/core/auth/initialize.ts b/packages-exp/auth-exp/src/core/auth/initialize.ts index 32e254ad84a..0e8b67efbec 100644 --- a/packages-exp/auth-exp/src/core/auth/initialize.ts +++ b/packages-exp/auth-exp/src/core/auth/initialize.ts @@ -16,6 +16,7 @@ */ import { _getProvider, FirebaseApp } from '@firebase/app-exp'; +import { deepEqual } from '@firebase/util'; import { Auth, Dependencies } from '../../model/public_types'; import { AuthErrorCode } from '../errors'; @@ -54,7 +55,16 @@ export function initializeAuth(app: FirebaseApp, deps?: Dependencies): Auth { if (provider.isInitialized()) { const auth = provider.getImmediate() as AuthImpl; - _fail(auth, AuthErrorCode.ALREADY_INITIALIZED); + const initialOptions = provider.getOptions() as Dependencies; + if ( + initialOptions.errorMap === deps?.errorMap && + initialOptions.popupRedirectResolver === deps?.popupRedirectResolver && + deepEqual(initialOptions.persistence as object, deps?.persistence as object) + ) { + return auth; + } else { + _fail(auth, AuthErrorCode.ALREADY_INITIALIZED); + } } const auth = provider.initialize({ options: deps }) as AuthImpl; @@ -67,9 +77,8 @@ export function _initializeAuthInstance( deps?: Dependencies ): void { const persistence = deps?.persistence || []; - const hierarchy = (Array.isArray(persistence) - ? persistence - : [persistence] + const hierarchy = ( + Array.isArray(persistence) ? persistence : [persistence] ).map(_getInstance); if (deps?.errorMap) { auth._updateErrorMap(deps.errorMap); From 0e704739bd81ec5d0a85ae613e93b6f502bdc55a Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Wed, 11 Aug 2021 14:07:48 -0700 Subject: [PATCH 2/7] prettier --- .../auth-exp/src/core/auth/initialize.test.ts | 47 ++++++++++++------- .../auth-exp/src/core/auth/initialize.ts | 5 +- 2 files changed, 34 insertions(+), 18 deletions(-) diff --git a/packages-exp/auth-exp/src/core/auth/initialize.test.ts b/packages-exp/auth-exp/src/core/auth/initialize.test.ts index e7d3e3d4f93..c72f27f0e1e 100644 --- a/packages-exp/auth-exp/src/core/auth/initialize.test.ts +++ b/packages-exp/auth-exp/src/core/auth/initialize.test.ts @@ -31,7 +31,11 @@ import { import { isNode } from '@firebase/util'; import { expect } from 'chai'; -import { browserLocalPersistence, browserSessionPersistence, inMemoryPersistence } from '../../../internal'; +import { + browserLocalPersistence, + browserSessionPersistence, + inMemoryPersistence +} from '../../../internal'; import { AuthInternal } from '../../model/auth'; import { @@ -128,7 +132,8 @@ describe('core/auth/initialize', () => { } } - const fakePopupRedirectResolver: PopupRedirectResolver = FakePopupRedirectResolver; + const fakePopupRedirectResolver: PopupRedirectResolver = + FakePopupRedirectResolver; before(() => { registerAuth(ClientPlatform.BROWSER); @@ -204,7 +209,7 @@ describe('core/auth/initialize', () => { const auth = initializeAuth(fakeApp, { popupRedirectResolver: fakePopupRedirectResolver }) as AuthInternal; - await ((auth as unknown) as _FirebaseService)._delete(); + await (auth as unknown as _FirebaseService)._delete(); await auth._initializationPromise; expect(auth._isInitialized).to.be.false; @@ -221,38 +226,46 @@ describe('core/auth/initialize', () => { persistence: browserSessionPersistence, popupRedirectResolver: fakePopupRedirectResolver }); - expect(initializeAuth(fakeApp, { - errorMap: prodErrorMap, - persistence: browserSessionPersistence, - popupRedirectResolver: fakePopupRedirectResolver - })).to.equal(auth); + expect( + initializeAuth(fakeApp, { + errorMap: prodErrorMap, + persistence: browserSessionPersistence, + popupRedirectResolver: fakePopupRedirectResolver + }) + ).to.equal(auth); }); it('should throw if called again with different params (popupRedirectResolver)', () => { initializeAuth(fakeApp, { popupRedirectResolver: fakePopupRedirectResolver }); - expect(() => initializeAuth(fakeApp, { - popupRedirectResolver: undefined - })).to.throw(); + expect(() => + initializeAuth(fakeApp, { + popupRedirectResolver: undefined + }) + ).to.throw(); }); it('should throw if called again with different params (errorMap)', () => { initializeAuth(fakeApp, { errorMap: prodErrorMap }); - expect(() => initializeAuth(fakeApp, { - errorMap: debugErrorMap - })).to.throw(); + expect(() => + initializeAuth(fakeApp, { + errorMap: debugErrorMap + }) + ).to.throw(); }); it('should throw if called again with different params (persistence)', () => { initializeAuth(fakeApp, { persistence: [browserLocalPersistence, browserSessionPersistence] }); - expect(() => initializeAuth(fakeApp, { - persistence: [browserSessionPersistence, browserLocalPersistence] - })).to.throw(); + expect(() => + initializeAuth(fakeApp, { + persistence: [browserSessionPersistence, browserLocalPersistence] + }) + ).to.throw(); }); }); }); diff --git a/packages-exp/auth-exp/src/core/auth/initialize.ts b/packages-exp/auth-exp/src/core/auth/initialize.ts index 0e8b67efbec..d9bcb3bd30f 100644 --- a/packages-exp/auth-exp/src/core/auth/initialize.ts +++ b/packages-exp/auth-exp/src/core/auth/initialize.ts @@ -59,7 +59,10 @@ export function initializeAuth(app: FirebaseApp, deps?: Dependencies): Auth { if ( initialOptions.errorMap === deps?.errorMap && initialOptions.popupRedirectResolver === deps?.popupRedirectResolver && - deepEqual(initialOptions.persistence as object, deps?.persistence as object) + deepEqual( + initialOptions.persistence as object, + deps?.persistence as object + ) ) { return auth; } else { From 5fd2e37040fc6ad62af95e607efab102ae9214e7 Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Wed, 11 Aug 2021 14:32:55 -0700 Subject: [PATCH 3/7] Update error message --- packages-exp/auth-exp/src/core/errors.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages-exp/auth-exp/src/core/errors.ts b/packages-exp/auth-exp/src/core/errors.ts index e955c4c1c0f..a79ecca7292 100644 --- a/packages-exp/auth-exp/src/core/errors.ts +++ b/packages-exp/auth-exp/src/core/errors.ts @@ -349,7 +349,10 @@ function _debugErrorMap(): ErrorMap { [AuthErrorCode.WEB_STORAGE_UNSUPPORTED]: 'This browser is not supported or 3rd party cookies and data may be disabled.', [AuthErrorCode.ALREADY_INITIALIZED]: - 'Auth can only be initialized once per app.' + 'initializeAuth() has already been called with ' + + 'different options. To avoid this error, call initializeAuth() with the ' + + 'same options as when it was originally called, or call getAuth() to return the' + + ' already initialized instance.' }; } From 576d21c32fd506b3ebc234b1dbc860c36b3638b4 Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Wed, 11 Aug 2021 15:14:41 -0700 Subject: [PATCH 4/7] Use test-safe fake persistences --- packages-exp/auth-exp/src/core/auth/initialize.test.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages-exp/auth-exp/src/core/auth/initialize.test.ts b/packages-exp/auth-exp/src/core/auth/initialize.test.ts index c72f27f0e1e..56072fd8980 100644 --- a/packages-exp/auth-exp/src/core/auth/initialize.test.ts +++ b/packages-exp/auth-exp/src/core/auth/initialize.test.ts @@ -32,7 +32,6 @@ import { isNode } from '@firebase/util'; import { expect } from 'chai'; import { - browserLocalPersistence, browserSessionPersistence, inMemoryPersistence } from '../../../internal'; @@ -259,11 +258,11 @@ describe('core/auth/initialize', () => { it('should throw if called again with different params (persistence)', () => { initializeAuth(fakeApp, { - persistence: [browserLocalPersistence, browserSessionPersistence] + persistence: [inMemoryPersistence, fakeSessionPersistence] }); expect(() => initializeAuth(fakeApp, { - persistence: [browserSessionPersistence, browserLocalPersistence] + persistence: [fakeSessionPersistence, inMemoryPersistence] }) ).to.throw(); }); From c6c254e79ea5d08e8bfc491a21b23532282660f4 Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Wed, 11 Aug 2021 15:58:06 -0700 Subject: [PATCH 5/7] Use deepEqual() --- packages-exp/auth-exp/src/core/auth/initialize.ts | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/packages-exp/auth-exp/src/core/auth/initialize.ts b/packages-exp/auth-exp/src/core/auth/initialize.ts index d9bcb3bd30f..8f8528969eb 100644 --- a/packages-exp/auth-exp/src/core/auth/initialize.ts +++ b/packages-exp/auth-exp/src/core/auth/initialize.ts @@ -56,14 +56,7 @@ export function initializeAuth(app: FirebaseApp, deps?: Dependencies): Auth { if (provider.isInitialized()) { const auth = provider.getImmediate() as AuthImpl; const initialOptions = provider.getOptions() as Dependencies; - if ( - initialOptions.errorMap === deps?.errorMap && - initialOptions.popupRedirectResolver === deps?.popupRedirectResolver && - deepEqual( - initialOptions.persistence as object, - deps?.persistence as object - ) - ) { + if (deepEqual(initialOptions, deps ?? {})) { return auth; } else { _fail(auth, AuthErrorCode.ALREADY_INITIALIZED); From e1e681dc57e5daca5e4283f692211272f168280b Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Wed, 11 Aug 2021 17:15:21 -0700 Subject: [PATCH 6/7] Missed a persistence --- packages-exp/auth-exp/src/core/auth/initialize.test.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages-exp/auth-exp/src/core/auth/initialize.test.ts b/packages-exp/auth-exp/src/core/auth/initialize.test.ts index 56072fd8980..cac22901755 100644 --- a/packages-exp/auth-exp/src/core/auth/initialize.test.ts +++ b/packages-exp/auth-exp/src/core/auth/initialize.test.ts @@ -32,7 +32,6 @@ import { isNode } from '@firebase/util'; import { expect } from 'chai'; import { - browserSessionPersistence, inMemoryPersistence } from '../../../internal'; @@ -222,13 +221,13 @@ describe('core/auth/initialize', () => { it('should not throw if called again with same params', () => { const auth = initializeAuth(fakeApp, { errorMap: prodErrorMap, - persistence: browserSessionPersistence, + persistence: fakeSessionPersistence, popupRedirectResolver: fakePopupRedirectResolver }); expect( initializeAuth(fakeApp, { errorMap: prodErrorMap, - persistence: browserSessionPersistence, + persistence: fakeSessionPersistence, popupRedirectResolver: fakePopupRedirectResolver }) ).to.equal(auth); From 7970f5029c2f154469f3e35753260c9b23b90c6a Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Wed, 11 Aug 2021 17:15:40 -0700 Subject: [PATCH 7/7] prettier --- packages-exp/auth-exp/src/core/auth/initialize.test.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages-exp/auth-exp/src/core/auth/initialize.test.ts b/packages-exp/auth-exp/src/core/auth/initialize.test.ts index cac22901755..7226d5b5374 100644 --- a/packages-exp/auth-exp/src/core/auth/initialize.test.ts +++ b/packages-exp/auth-exp/src/core/auth/initialize.test.ts @@ -31,9 +31,7 @@ import { import { isNode } from '@firebase/util'; import { expect } from 'chai'; -import { - inMemoryPersistence -} from '../../../internal'; +import { inMemoryPersistence } from '../../../internal'; import { AuthInternal } from '../../model/auth'; import {