From bacb70310848d98cac9c9f1f6cbe0c0f3bc33ae2 Mon Sep 17 00:00:00 2001 From: Feiyang1 Date: Wed, 22 Jul 2020 12:26:03 -0700 Subject: [PATCH 01/10] delete services that implement the next FirebaseService interface in @firebase/component correctly --- packages-exp/app-exp/src/api.test.ts | 32 ++++++++++++++++++++++++- packages/component/src/provider.test.ts | 26 +++++++++++++++++++- packages/component/src/provider.ts | 14 +++++++---- 3 files changed, 65 insertions(+), 7 deletions(-) diff --git a/packages-exp/app-exp/src/api.test.ts b/packages-exp/app-exp/src/api.test.ts index 307ab4b621f..d26649514d5 100644 --- a/packages-exp/app-exp/src/api.test.ts +++ b/packages-exp/app-exp/src/api.test.ts @@ -29,7 +29,10 @@ import { onLog } from './api'; import { DEFAULT_ENTRY_NAME } from './constants'; -import { _FirebaseAppInternal } from '@firebase/app-types-exp'; +import { + _FirebaseAppInternal, + _FirebaseService +} from '@firebase/app-types-exp'; import { _clearComponents, _components, @@ -175,6 +178,33 @@ describe('API tests', () => { deleteApp(app).catch(() => {}); expect(getApps().length).to.equal(0); }); + + it('waits for all services being deleted', async () => { + _clearComponents(); + let count = 0; + const comp1 = new Component( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + 'test1' as any, + _container => + ({ + delete: async () => { + await Promise.resolve(); + expect(count).to.equal(0); + count++; + } + } as _FirebaseService), + ComponentType.PUBLIC + ); + _registerComponent(comp1); + + const app = initializeApp({}); + // create service instance + const test1Provider = _getProvider(app, 'test1' as any); + test1Provider.getImmediate(); + + await deleteApp(app); + expect(count).to.equal(1); + }); }); describe('registerVersion', () => { diff --git a/packages/component/src/provider.test.ts b/packages/component/src/provider.test.ts index 253b9942f5b..98b48af2931 100644 --- a/packages/component/src/provider.test.ts +++ b/packages/component/src/provider.test.ts @@ -19,6 +19,7 @@ import { expect } from 'chai'; import { fake, SinonSpy } from 'sinon'; import { ComponentContainer } from './component_container'; import { FirebaseService } from '@firebase/app-types/private'; +import { _FirebaseService } from '@firebase/app-types-exp'; import { Provider } from './provider'; import { getFakeApp, getFakeComponent } from '../test/util'; import '../test/setup'; @@ -202,7 +203,7 @@ describe('Provider', () => { }); describe('delete()', () => { - it('calls delete() on the service instance that implements FirebaseService', () => { + it('calls delete() on the service instance that implements legacy FirebaseService', () => { const deleteFake = fake(); const myService: FirebaseService = { app: getFakeApp(), @@ -226,6 +227,29 @@ describe('Provider', () => { expect(deleteFake).to.have.been.called; }); + + it('calls delete() on the service instance that implements next FirebaseService', () => { + const deleteFake = fake(); + const myService: _FirebaseService = { + app: getFakeApp(), + delete: deleteFake + }; + + // provide factory and create a service instance + provider.setComponent( + getFakeComponent( + 'test', + () => myService, + false, + InstantiationMode.EAGER + ) + ); + + // eslint-disable-next-line @typescript-eslint/no-floating-promises + provider.delete(); + + expect(deleteFake).to.have.been.called; + }); }); describe('clearCache()', () => { diff --git a/packages/component/src/provider.ts b/packages/component/src/provider.ts index 14bd0de092a..2f61c07d2c7 100644 --- a/packages/component/src/provider.ts +++ b/packages/component/src/provider.ts @@ -170,12 +170,16 @@ export class Provider { async delete(): Promise { const services = Array.from(this.instances.values()); - await Promise.all( - services - .filter(service => 'INTERNAL' in service) + await Promise.all([ + ...services + .filter(service => 'INTERNAL' in service) // legacy services // eslint-disable-next-line @typescript-eslint/no-explicit-any - .map(service => (service as any).INTERNAL!.delete()) - ); + .map(service => (service as any).INTERNAL!.delete()), + ...services + .filter(service => 'delete' in service) // next services + // eslint-disable-next-line @typescript-eslint/no-explicit-any + .map(service => (service as any).delete()) + ]); } isComponentSet(): boolean { From 7eebd5f18830f62da701b5f422d7aa2e94296dc0 Mon Sep 17 00:00:00 2001 From: Feiyang Date: Wed, 22 Jul 2020 12:29:54 -0700 Subject: [PATCH 02/10] Create small-grapes-teach.md --- .changeset/small-grapes-teach.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/small-grapes-teach.md diff --git a/.changeset/small-grapes-teach.md b/.changeset/small-grapes-teach.md new file mode 100644 index 00000000000..467bd2b7f10 --- /dev/null +++ b/.changeset/small-grapes-teach.md @@ -0,0 +1,5 @@ +--- +"@firebase/component": patch +--- + +Correctly delete services that implement the new FirebaseService interface when calling provider.delete() From 4617b3c8db7b3543367925f011f4af5a58f57be1 Mon Sep 17 00:00:00 2001 From: Feiyang1 Date: Tue, 11 Aug 2020 20:46:23 -0700 Subject: [PATCH 03/10] debug CI issue --- packages/component/src/provider.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/component/src/provider.ts b/packages/component/src/provider.ts index 2f61c07d2c7..409049e8969 100644 --- a/packages/component/src/provider.ts +++ b/packages/component/src/provider.ts @@ -174,11 +174,11 @@ export class Provider { ...services .filter(service => 'INTERNAL' in service) // legacy services // eslint-disable-next-line @typescript-eslint/no-explicit-any - .map(service => (service as any).INTERNAL!.delete()), - ...services - .filter(service => 'delete' in service) // next services - // eslint-disable-next-line @typescript-eslint/no-explicit-any - .map(service => (service as any).delete()) + .map(service => (service as any).INTERNAL!.delete()) + // ...services + // .filter(service => 'delete' in service) // next services + // // eslint-disable-next-line @typescript-eslint/no-explicit-any + // .map(service => (service as any).delete()) ]); } From ff5b78eb2452c2de8c176329d41e51d122878244 Mon Sep 17 00:00:00 2001 From: Feiyang1 Date: Tue, 11 Aug 2020 21:58:11 -0700 Subject: [PATCH 04/10] fix the delete loop --- packages/component/src/provider.ts | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/packages/component/src/provider.ts b/packages/component/src/provider.ts index 409049e8969..175ec278e6d 100644 --- a/packages/component/src/provider.ts +++ b/packages/component/src/provider.ts @@ -174,11 +174,20 @@ export class Provider { ...services .filter(service => 'INTERNAL' in service) // legacy services // eslint-disable-next-line @typescript-eslint/no-explicit-any - .map(service => (service as any).INTERNAL!.delete()) - // ...services - // .filter(service => 'delete' in service) // next services - // // eslint-disable-next-line @typescript-eslint/no-explicit-any - // .map(service => (service as any).delete()) + .map(service => (service as any).INTERNAL!.delete()), + ...services + // eslint-disable-next-line @typescript-eslint/no-explicit-any + .filter( + service => + 'delete' in service && + this.component && + // TODO: remove !== 'app' when modular SDKs become official + // People call app.delete() to trigger provider.delete() for all registered components, so + // we don't call delete() on legacy FirebaseApp to avoid getting into a loop. + (this.component.name as any) !== 'app' + ) // modular services + // eslint-disable-next-line @typescript-eslint/no-explicit-any + .map(service => (service as any).delete()) ]); } From 2df6b21645c456132c933f308de4c72cfaa814c2 Mon Sep 17 00:00:00 2001 From: Feiyang1 Date: Tue, 11 Aug 2020 22:22:41 -0700 Subject: [PATCH 05/10] add eslint suppress --- packages/component/src/provider.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/component/src/provider.ts b/packages/component/src/provider.ts index 175ec278e6d..6ca123f5a14 100644 --- a/packages/component/src/provider.ts +++ b/packages/component/src/provider.ts @@ -176,7 +176,6 @@ export class Provider { // eslint-disable-next-line @typescript-eslint/no-explicit-any .map(service => (service as any).INTERNAL!.delete()), ...services - // eslint-disable-next-line @typescript-eslint/no-explicit-any .filter( service => 'delete' in service && @@ -184,6 +183,7 @@ export class Provider { // TODO: remove !== 'app' when modular SDKs become official // People call app.delete() to trigger provider.delete() for all registered components, so // we don't call delete() on legacy FirebaseApp to avoid getting into a loop. + // eslint-disable-next-line @typescript-eslint/no-explicit-any (this.component.name as any) !== 'app' ) // modular services // eslint-disable-next-line @typescript-eslint/no-explicit-any From f1b0b4422394810cf7c4290eb91392b211700cf1 Mon Sep 17 00:00:00 2001 From: Feiyang1 Date: Wed, 12 Aug 2020 13:06:00 -0700 Subject: [PATCH 06/10] test --- packages/analytics/src/factory.ts | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/packages/analytics/src/factory.ts b/packages/analytics/src/factory.ts index 3335f9b03c7..81c01161926 100644 --- a/packages/analytics/src/factory.ts +++ b/packages/analytics/src/factory.ts @@ -123,21 +123,21 @@ export function factory( app: FirebaseApp, installations: FirebaseInstallations ): FirebaseAnalytics { - if (isBrowserExtension()) { - throw ERROR_FACTORY.create(AnalyticsError.INVALID_ANALYTICS_CONTEXT); - } - if (!areCookiesEnabled()) { - throw ERROR_FACTORY.create(AnalyticsError.COOKIES_NOT_ENABLED); - } - if (!isIndexedDBAvailable()) { - throw ERROR_FACTORY.create(AnalyticsError.INDEXED_DB_UNSUPPORTED); - } - // Async but non-blocking. - validateIndexedDBOpenable().catch(error => { - throw ERROR_FACTORY.create(AnalyticsError.INVALID_INDEXED_DB_CONTEXT, { - errorInfo: error - }); - }); + // if (isBrowserExtension()) { + // throw ERROR_FACTORY.create(AnalyticsError.INVALID_ANALYTICS_CONTEXT); + // } + // if (!areCookiesEnabled()) { + // throw ERROR_FACTORY.create(AnalyticsError.COOKIES_NOT_ENABLED); + // } + // if (!isIndexedDBAvailable()) { + // throw ERROR_FACTORY.create(AnalyticsError.INDEXED_DB_UNSUPPORTED); + // } + // // Async but non-blocking. + // validateIndexedDBOpenable().catch(error => { + // throw ERROR_FACTORY.create(AnalyticsError.INVALID_INDEXED_DB_CONTEXT, { + // errorInfo: error + // }); + // }); const analyticsId = app.options[ANALYTICS_ID_FIELD]; if (!analyticsId) { From 57caf9184b08ff955194cdb6adbd208fbd5c3eb8 Mon Sep 17 00:00:00 2001 From: Feiyang1 Date: Wed, 12 Aug 2020 13:35:36 -0700 Subject: [PATCH 07/10] comment out imports --- packages/analytics/src/factory.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/analytics/src/factory.ts b/packages/analytics/src/factory.ts index 81c01161926..b931b8fe698 100644 --- a/packages/analytics/src/factory.ts +++ b/packages/analytics/src/factory.ts @@ -38,12 +38,12 @@ import { ANALYTICS_ID_FIELD } from './constants'; import { AnalyticsError, ERROR_FACTORY } from './errors'; import { FirebaseApp } from '@firebase/app-types'; import { FirebaseInstallations } from '@firebase/installations-types'; -import { - isIndexedDBAvailable, - validateIndexedDBOpenable, - areCookiesEnabled, - isBrowserExtension -} from '@firebase/util'; +// import { +// isIndexedDBAvailable, +// validateIndexedDBOpenable, +// areCookiesEnabled, +// isBrowserExtension +// } from '@firebase/util'; /** * Maps gaId to FID fetch promises. From 8ba181608c24ddc2579ad3ba39476af598ffef4b Mon Sep 17 00:00:00 2001 From: Feiyang1 Date: Thu, 13 Aug 2020 09:58:23 -0700 Subject: [PATCH 08/10] Revert "test" This reverts commit f1b0b4422394810cf7c4290eb91392b211700cf1. --- packages/analytics/src/factory.ts | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/packages/analytics/src/factory.ts b/packages/analytics/src/factory.ts index b931b8fe698..d04fcaef5b3 100644 --- a/packages/analytics/src/factory.ts +++ b/packages/analytics/src/factory.ts @@ -123,21 +123,21 @@ export function factory( app: FirebaseApp, installations: FirebaseInstallations ): FirebaseAnalytics { - // if (isBrowserExtension()) { - // throw ERROR_FACTORY.create(AnalyticsError.INVALID_ANALYTICS_CONTEXT); - // } - // if (!areCookiesEnabled()) { - // throw ERROR_FACTORY.create(AnalyticsError.COOKIES_NOT_ENABLED); - // } - // if (!isIndexedDBAvailable()) { - // throw ERROR_FACTORY.create(AnalyticsError.INDEXED_DB_UNSUPPORTED); - // } - // // Async but non-blocking. - // validateIndexedDBOpenable().catch(error => { - // throw ERROR_FACTORY.create(AnalyticsError.INVALID_INDEXED_DB_CONTEXT, { - // errorInfo: error - // }); - // }); + if (isBrowserExtension()) { + throw ERROR_FACTORY.create(AnalyticsError.INVALID_ANALYTICS_CONTEXT); + } + if (!areCookiesEnabled()) { + throw ERROR_FACTORY.create(AnalyticsError.COOKIES_NOT_ENABLED); + } + if (!isIndexedDBAvailable()) { + throw ERROR_FACTORY.create(AnalyticsError.INDEXED_DB_UNSUPPORTED); + } + // Async but non-blocking. + validateIndexedDBOpenable().catch(error => { + throw ERROR_FACTORY.create(AnalyticsError.INVALID_INDEXED_DB_CONTEXT, { + errorInfo: error + }); + }); const analyticsId = app.options[ANALYTICS_ID_FIELD]; if (!analyticsId) { From 7a2a71f459005cd5d856fe6e104867605c0cf92f Mon Sep 17 00:00:00 2001 From: Feiyang1 Date: Thu, 13 Aug 2020 09:58:35 -0700 Subject: [PATCH 09/10] Revert "comment out imports" This reverts commit 57caf9184b08ff955194cdb6adbd208fbd5c3eb8. --- packages/analytics/src/factory.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/analytics/src/factory.ts b/packages/analytics/src/factory.ts index d04fcaef5b3..3335f9b03c7 100644 --- a/packages/analytics/src/factory.ts +++ b/packages/analytics/src/factory.ts @@ -38,12 +38,12 @@ import { ANALYTICS_ID_FIELD } from './constants'; import { AnalyticsError, ERROR_FACTORY } from './errors'; import { FirebaseApp } from '@firebase/app-types'; import { FirebaseInstallations } from '@firebase/installations-types'; -// import { -// isIndexedDBAvailable, -// validateIndexedDBOpenable, -// areCookiesEnabled, -// isBrowserExtension -// } from '@firebase/util'; +import { + isIndexedDBAvailable, + validateIndexedDBOpenable, + areCookiesEnabled, + isBrowserExtension +} from '@firebase/util'; /** * Maps gaId to FID fetch promises. From b7dc4fff8cbe222067c78ea2825d82ce10287e7a Mon Sep 17 00:00:00 2001 From: Feiyang Date: Thu, 13 Aug 2020 16:22:04 -0700 Subject: [PATCH 10/10] Update small-grapes-teach.md --- .changeset/small-grapes-teach.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/small-grapes-teach.md b/.changeset/small-grapes-teach.md index 467bd2b7f10..5a15dfe3552 100644 --- a/.changeset/small-grapes-teach.md +++ b/.changeset/small-grapes-teach.md @@ -2,4 +2,4 @@ "@firebase/component": patch --- -Correctly delete services that implement the new FirebaseService interface when calling provider.delete() +Correctly delete services created by modular SDKs when calling provider.delete()