From 33acd67002037780e0494ddb3bb99a1b275965d1 Mon Sep 17 00:00:00 2001 From: Feiyang1 Date: Tue, 27 Jul 2021 16:30:36 -0700 Subject: [PATCH 1/5] Add deepEqual function --- packages-exp/app-exp/src/api.ts | 2 + packages-exp/app-exp/src/firebaseApp.ts | 13 ++ packages/util/src/obj.ts | 38 ++++ packages/util/test/object.test.ts | 248 ++++++++++++++++++++++++ 4 files changed, 301 insertions(+) create mode 100644 packages/util/test/object.test.ts diff --git a/packages-exp/app-exp/src/api.ts b/packages-exp/app-exp/src/api.ts index 2a7691ab1ce..b52edce668e 100644 --- a/packages-exp/app-exp/src/api.ts +++ b/packages-exp/app-exp/src/api.ts @@ -130,6 +130,8 @@ export function initializeApp( } if (_apps.has(name)) { + // return the existing app if options and config deep equal the ones in the existing app. + throw ERROR_FACTORY.create(AppError.DUPLICATE_APP, { appName: name }); } diff --git a/packages-exp/app-exp/src/firebaseApp.ts b/packages-exp/app-exp/src/firebaseApp.ts index db22f2db255..7333a76f22d 100644 --- a/packages-exp/app-exp/src/firebaseApp.ts +++ b/packages-exp/app-exp/src/firebaseApp.ts @@ -30,6 +30,13 @@ import { ERROR_FACTORY, AppError } from './errors'; export class FirebaseAppImpl implements FirebaseApp { private readonly _options: FirebaseOptions; private readonly _name: string; + /** + * Original config values passed in as a constructor parameter. + * It is only used to compare with another config object to support idempotent initializeApp(). + * + * Updating automaticDataCollectionEnabled on the App instance will not change its value in _config. + */ + private readonly _config: Required; private _automaticDataCollectionEnabled: boolean; private _isDeleted = false; private readonly _container: ComponentContainer; @@ -40,6 +47,7 @@ export class FirebaseAppImpl implements FirebaseApp { container: ComponentContainer ) { this._options = { ...options }; + this._config = { ...config }; this._name = config.name; this._automaticDataCollectionEnabled = config.automaticDataCollectionEnabled; @@ -69,6 +77,11 @@ export class FirebaseAppImpl implements FirebaseApp { return this._options; } + get config(): Required { + this.checkDestroyed(); + return this._config; + } + get container(): ComponentContainer { return this._container; } diff --git a/packages/util/src/obj.ts b/packages/util/src/obj.ts index 3071d50e004..fc3762ebb3c 100644 --- a/packages/util/src/obj.ts +++ b/packages/util/src/obj.ts @@ -52,3 +52,41 @@ export function map( } return res as { [key in K]: U }; } + +/** + * Deep equal two objects. Support Arrays and Objects. + */ +export function deepEqual(a: object, b: object): boolean { + if (a === b) { + return true; + } + + const aKeys = Object.keys(a); + const bKeys = Object.keys(b); + for (const k of aKeys) { + if (!bKeys.includes(k)) { + return false; + } + + const aProp = (a as Record)[k]; + const bProp = (b as Record)[k]; + if (isObject(aProp) && isObject(bProp)) { + if (!deepEqual(aProp, bProp)) { + return false; + } + } else if (aProp !== bProp) { + return false; + } + } + + for (const k of bKeys) { + if (!aKeys.includes(k)) { + return false; + } + } + return true; +} + +function isObject(thing: unknown): thing is object { + return thing !== null && typeof thing === 'object'; +} diff --git a/packages/util/test/object.test.ts b/packages/util/test/object.test.ts new file mode 100644 index 00000000000..d6f86209a83 --- /dev/null +++ b/packages/util/test/object.test.ts @@ -0,0 +1,248 @@ +/** + * @license + * Copyright 2021 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { expect } from 'chai'; +import { deepEqual } from '../src/obj'; + +// eslint-disable-next-line no-restricted-properties +describe.only('deepEqual()', () => { + it('returns true for comparing empty objects', () => { + expect(deepEqual({}, {})).to.be.true; + }); + + it('returns true for comparing the same object', () => { + const obj = { + field1: 1 + }; + + expect(deepEqual(obj, obj)).to.be.true; + }); + + it('returns true for comparing shallow identical objects', () => { + const obj1 = { + field1: 1, + field2: { + nested1: 1 + } + }; + + const obj2 = { + ...obj1 + }; + + expect(deepEqual(obj1, obj2)).to.be.true; + }); + + it('returns true for deeply nested identical objects', () => { + const obj1 = { + field1: 1, + field2: { + nested1: { + nested2: 2, + nested3: 3, + nested4: { + nested5: 5 + } + } + }, + field3: { + nested1: { + nested2: 2, + nested3: 3, + nested4: { + nested5: 5 + } + } + } + }; + + const obj2 = { + field1: 1, + field2: { + nested1: { + nested2: 2, + nested3: 3, + nested4: { + nested5: 5 + } + } + }, + field3: { + nested1: { + nested2: 2, + nested3: 3, + nested4: { + nested5: 5 + } + } + } + }; + + expect(deepEqual(obj1, obj2)).to.be.true; + }); + + it('returns true for comparing identical arrays', () => { + const arr1 = [1, 2, 3, 4]; + const arr2 = [1, 2, 3, 4]; + + expect(deepEqual(arr1, arr2)).to.be.true; + }); + + it('returns true for comparing arrays with nested objects', () => { + const arr1 = [ + 1, + 2, + { + field1: 1, + field2: { + nested1: { + nested2: 2, + nested3: 3, + nested4: { + nested5: 5 + } + } + }, + field3: { + nested1: { + nested2: 2, + nested3: 3, + nested4: { + nested5: 5 + } + } + } + } + ]; + + const arr2 = [ + 1, + 2, + { + field1: 1, + field2: { + nested1: { + nested2: 2, + nested3: 3, + nested4: { + nested5: 5 + } + } + }, + field3: { + nested1: { + nested2: 2, + nested3: 3, + nested4: { + nested5: 5 + } + } + } + } + ]; + + expect(deepEqual(arr1, arr2)).to.be.true; + }); + + it('returns false for shallowly different objects', () => { + const obj1 = { + field1: 1, + field2: 2 + }; + + const obj2 = { + field1: 1, + field2: '2' + }; + + expect(deepEqual(obj1, obj2)).to.be.false; + }); + + it('returns false for different objects with nesting', () => { + const obj1 = { + field1: 1, + field2: { + nested1: { + nested2: 2, + nested3: 3, + nested4: { + nested5: 5 + } + } + }, + field3: { + nested1: { + nested2: 2, + nested3: 3, + nested4: { + nested5: 5 + } + } + } + }; + + const obj2 = { + field1: 1, + field2: { + nested1: { + nested2: 2, + nested3: 3, + nested4: { + nested5: '5' + } + } + }, + field3: { + nested1: { + nested2: '2', + nested3: 3, + nested4: { + nested5: 5 + } + } + } + }; + + expect(deepEqual(obj1, obj2)).to.be.false; + }); + + it('handles undefined & null properties correctly', () => { + const obj1 = { + field1: undefined, + field2: null + }; + const obj2 = { + field1: undefined, + field2: null + }; + expect(deepEqual(obj1, obj2)).to.be.true; + + const obj3 = { + field1: undefined + }; + const obj4 = { + field1: null + }; + expect(deepEqual(obj3, obj4)).to.be.false; + + const obj5 = { + field1: undefined + }; + const obj6 = {}; + expect(deepEqual(obj5, obj6)).to.be.false; + }); +}); From 69a2fc199da5b3d4944b66065ce76f03170ec8a7 Mon Sep 17 00:00:00 2001 From: Feiyang1 Date: Tue, 27 Jul 2021 17:04:37 -0700 Subject: [PATCH 2/5] make initializeApp idempotent --- packages-exp/app-exp/src/api.test.ts | 82 ++++++++++++++++++++++++++-- packages-exp/app-exp/src/api.ts | 14 ++++- packages-exp/app-exp/src/errors.ts | 3 +- 3 files changed, 89 insertions(+), 10 deletions(-) diff --git a/packages-exp/app-exp/src/api.test.ts b/packages-exp/app-exp/src/api.test.ts index 9c0770e1a59..1f704943598 100644 --- a/packages-exp/app-exp/src/api.test.ts +++ b/packages-exp/app-exp/src/api.test.ts @@ -74,15 +74,85 @@ describe('API tests', () => { expect(app2.name).to.equal(appName); }); - it('throws when creating duplicate DEDAULT Apps', () => { - initializeApp({}); - expect(() => initializeApp({})).throws(/\[DEFAULT\].*exists/i); + it('can be called more than once and returns the same instance if the options and config are the same', () => { + const app = initializeApp( + { + apiKey: 'test1' + }, + { automaticDataCollectionEnabled: true } + ); + expect( + initializeApp( + { + apiKey: 'test1' + }, + { automaticDataCollectionEnabled: true } + ) + ).to.equal(app); + }); + + it('throws when creating duplicate DEDAULT Apps with different options', () => { + initializeApp({ + apiKey: 'test1' + }); + expect(() => + initializeApp({ + apiKey: 'test2' + }) + ).throws(/\[DEFAULT\].*exists/i); + }); + + it('throws when creating duplicate named Apps with different options', () => { + const appName = 'MyApp'; + initializeApp( + { + apiKey: 'test1' + }, + appName + ); + expect(() => + initializeApp( + { + apiKey: 'test2' + }, + appName + ) + ).throws(/'MyApp'.*exists/i); + }); + + it('throws when creating duplicate DEDAULT Apps with different config values', () => { + initializeApp( + { + apiKey: 'test1' + }, + { automaticDataCollectionEnabled: true } + ); + expect(() => + initializeApp( + { + apiKey: 'test1' + }, + { automaticDataCollectionEnabled: false } + ) + ).throws(/\[DEFAULT\].*exists/i); }); - it('throws when creating duplicate named Apps', () => { + it('throws when creating duplicate named Apps with different config values', () => { const appName = 'MyApp'; - initializeApp({}, appName); - expect(() => initializeApp({}, appName)).throws(/'MyApp'.*exists/i); + initializeApp( + { + apiKey: 'test1' + }, + { name: appName, automaticDataCollectionEnabled: true } + ); + expect(() => + initializeApp( + { + apiKey: 'test1' + }, + { name: appName, automaticDataCollectionEnabled: false } + ) + ).throws(/'MyApp'.*exists/i); }); it('takes an object as the second parameter to create named App', () => { diff --git a/packages-exp/app-exp/src/api.ts b/packages-exp/app-exp/src/api.ts index b52edce668e..79f2cbf3fee 100644 --- a/packages-exp/app-exp/src/api.ts +++ b/packages-exp/app-exp/src/api.ts @@ -39,6 +39,7 @@ import { LogOptions, setUserLogHandler } from '@firebase/logger'; +import { deepEqual } from '@firebase/util'; /** * The current SDK version. @@ -129,10 +130,17 @@ export function initializeApp( }); } - if (_apps.has(name)) { + const existingApp = _apps.get(name) as FirebaseAppImpl; + if (existingApp) { // return the existing app if options and config deep equal the ones in the existing app. - - throw ERROR_FACTORY.create(AppError.DUPLICATE_APP, { appName: name }); + if ( + deepEqual(options, existingApp.options) && + deepEqual(config, existingApp.config) + ) { + return existingApp; + } else { + throw ERROR_FACTORY.create(AppError.DUPLICATE_APP, { appName: name }); + } } const container = new ComponentContainer(name); diff --git a/packages-exp/app-exp/src/errors.ts b/packages-exp/app-exp/src/errors.ts index 80bbf7082e4..a8aaca17554 100644 --- a/packages-exp/app-exp/src/errors.ts +++ b/packages-exp/app-exp/src/errors.ts @@ -31,7 +31,8 @@ const ERRORS: ErrorMap = { "No Firebase App '{$appName}' has been created - " + 'call Firebase App.initializeApp()', [AppError.BAD_APP_NAME]: "Illegal App name: '{$appName}", - [AppError.DUPLICATE_APP]: "Firebase App named '{$appName}' already exists", + [AppError.DUPLICATE_APP]: + "Firebase App named '{$appName}' already exists with different options", [AppError.APP_DELETED]: "Firebase App named '{$appName}' already deleted", [AppError.INVALID_APP_ARGUMENT]: 'firebase.{$appName}() takes either no argument or a ' + From 597b79ccf04a65093686a7a9f79da36795dc927e Mon Sep 17 00:00:00 2001 From: Feiyang1 Date: Tue, 27 Jul 2021 17:29:53 -0700 Subject: [PATCH 3/5] make initializeApp in app-compat idempotent --- .../app-compat/src/firebaseNamespaceCore.ts | 7 +++ .../app-compat/test/firebaseAppCompat.test.ts | 58 +++++++++++++++++-- 2 files changed, 59 insertions(+), 6 deletions(-) diff --git a/packages-exp/app-compat/src/firebaseNamespaceCore.ts b/packages-exp/app-compat/src/firebaseNamespaceCore.ts index f16b4b5bb21..e5d96409ae5 100644 --- a/packages-exp/app-compat/src/firebaseNamespaceCore.ts +++ b/packages-exp/app-compat/src/firebaseNamespaceCore.ts @@ -109,6 +109,8 @@ export function createFirebaseNamespaceCore( /** * Create a new App instance (name must be unique). + * + * This function is idempotent. It can be called more than once and return the same instance using the same options and config. */ function initializeAppCompat( options: FirebaseOptions, @@ -118,6 +120,11 @@ export function createFirebaseNamespaceCore( options, rawConfig ) as _FirebaseAppExp; + + if (contains(apps, app.name)) { + return apps[app.name]; + } + const appCompat = new firebaseAppImpl(app, namespace); apps[app.name] = appCompat; return appCompat; diff --git a/packages-exp/app-compat/test/firebaseAppCompat.test.ts b/packages-exp/app-compat/test/firebaseAppCompat.test.ts index 1e6cbf034c4..4803d9dffd2 100644 --- a/packages-exp/app-compat/test/firebaseAppCompat.test.ts +++ b/packages-exp/app-compat/test/firebaseAppCompat.test.ts @@ -346,15 +346,61 @@ function firebaseAppTests( expect(firebase.apps.length).to.eq(2); }); - it('duplicate DEFAULT initialize is an error.', () => { - firebase.initializeApp({}); - expect(() => firebase.initializeApp({})).throws(/\[DEFAULT\].*exists/i); + it('initilizeApp can be called more than once and returns the same instance if the options and config are the same', () => { + const app = firebase.initializeApp( + { + apiKey: 'test1' + }, + { automaticDataCollectionEnabled: true } + ); + expect( + firebase.initializeApp( + { + apiKey: 'test1' + }, + { automaticDataCollectionEnabled: true } + ) + ).to.equal(app); + }); + + it('duplicate DEFAULT initialize with different options is an error.', () => { + firebase.initializeApp({ apiKey: 'key1' }); + expect(() => firebase.initializeApp({ apiKey: 'key2' })).throws( + /\[DEFAULT\].*exists/i + ); + }); + + it('duplicate named App initialize with different options is an error.', () => { + firebase.initializeApp({ apiKey: 'key1', appId: 'id' }, 'abc'); + expect(() => firebase.initializeApp({ apiKey: 'key1' }, 'abc')).throws( + /'abc'.*exists/i + ); }); - it('duplicate named App initialize is an error.', () => { - firebase.initializeApp({}, 'abc'); + it('duplicate DEFAULT initialize with different config is an error.', () => { + firebase.initializeApp( + { apiKey: 'key1' }, + { automaticDataCollectionEnabled: true } + ); + expect(() => + firebase.initializeApp( + { apiKey: 'key1' }, + { automaticDataCollectionEnabled: false } + ) + ).throws(/\[DEFAULT\].*exists/i); + }); - expect(() => firebase.initializeApp({}, 'abc')).throws(/'abc'.*exists/i); + it('duplicate named App initialize with different config is an error.', () => { + firebase.initializeApp( + { apiKey: 'key1' }, + { name: 'abc', automaticDataCollectionEnabled: true } + ); + expect(() => + firebase.initializeApp( + { apiKey: 'key1' }, + { name: 'abc', automaticDataCollectionEnabled: false } + ) + ).throws(/'abc'.*exists/i); }); it('automaticDataCollectionEnabled is `false` by default', () => { From 170f9b070b7b5824cab89e53118a94989e7a8868 Mon Sep 17 00:00:00 2001 From: Feiyang Date: Tue, 27 Jul 2021 17:35:56 -0700 Subject: [PATCH 4/5] Create fair-readers-drum.md --- .changeset/fair-readers-drum.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/fair-readers-drum.md diff --git a/.changeset/fair-readers-drum.md b/.changeset/fair-readers-drum.md new file mode 100644 index 00000000000..97ef0fd8b4f --- /dev/null +++ b/.changeset/fair-readers-drum.md @@ -0,0 +1,5 @@ +--- +"@firebase/util": minor +--- + +Added deepEqual for comparing objects From 080aa0fe4f554320b619d417a053b34557eb9ca2 Mon Sep 17 00:00:00 2001 From: Feiyang1 Date: Wed, 28 Jul 2021 10:40:35 -0700 Subject: [PATCH 5/5] address comments --- packages-exp/app-compat/test/firebaseAppCompat.test.ts | 2 +- packages-exp/app-exp/src/errors.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages-exp/app-compat/test/firebaseAppCompat.test.ts b/packages-exp/app-compat/test/firebaseAppCompat.test.ts index 4803d9dffd2..83c0367f569 100644 --- a/packages-exp/app-compat/test/firebaseAppCompat.test.ts +++ b/packages-exp/app-compat/test/firebaseAppCompat.test.ts @@ -346,7 +346,7 @@ function firebaseAppTests( expect(firebase.apps.length).to.eq(2); }); - it('initilizeApp can be called more than once and returns the same instance if the options and config are the same', () => { + it('initializeApp can be called more than once and returns the same instance if the options and config are the same', () => { const app = firebase.initializeApp( { apiKey: 'test1' diff --git a/packages-exp/app-exp/src/errors.ts b/packages-exp/app-exp/src/errors.ts index a8aaca17554..b8bbae5c1b8 100644 --- a/packages-exp/app-exp/src/errors.ts +++ b/packages-exp/app-exp/src/errors.ts @@ -32,7 +32,7 @@ const ERRORS: ErrorMap = { 'call Firebase App.initializeApp()', [AppError.BAD_APP_NAME]: "Illegal App name: '{$appName}", [AppError.DUPLICATE_APP]: - "Firebase App named '{$appName}' already exists with different options", + "Firebase App named '{$appName}' already exists with different options or config", [AppError.APP_DELETED]: "Firebase App named '{$appName}' already deleted", [AppError.INVALID_APP_ARGUMENT]: 'firebase.{$appName}() takes either no argument or a ' +