From 6456f327113bca163693ca10151e50ae6df3f5a3 Mon Sep 17 00:00:00 2001 From: Armando Navarro Date: Fri, 30 Oct 2026 07:33:50 -0700 Subject: [PATCH 1/3] fix(core): wrap a handler passed as an observer object like a function Firebase accepts a subscription handler either as a function or as an object with next, error and complete members (data-connect spells them onNext, onErr, onComplete). The zone wrapper only recognized the function, so an observer object reached the SDK untouched: no pending task held server rendering, inject() threw inside the handler, and in a zone.js app the handler ran outside the Angular zone, where the wrapper itself had moved the call. The object is now replaced by a copy whose function members are wrapped the same way, with this bound to the caller's object. onConfigUpdate takes only the object form and its handler fires only on a remote template change, so it joins the blockUntilFirst: false list along with onTokenChanged, whose first callback a first visit may never request. Both would otherwise hold the application unstable forever once their observers are wrapped. --- src/app-check/firebase.ts | 2 +- src/remote-config/firebase.ts | 2 +- src/zones.spec.ts | 246 ++++++++++++++++++++++++++++++++++ src/zones.ts | 44 ++++-- tools/build.ts | 7 +- 5 files changed, 290 insertions(+), 11 deletions(-) create mode 100644 src/zones.spec.ts diff --git a/src/app-check/firebase.ts b/src/app-check/firebase.ts index bfaefcc29..e7928dafb 100644 --- a/src/app-check/firebase.ts +++ b/src/app-check/firebase.ts @@ -12,5 +12,5 @@ import { export const getLimitedUseToken = ɵzoneWrap(_getLimitedUseToken, true, 2); export const getToken = ɵzoneWrap(_getToken, true); export const initializeAppCheck = ɵzoneWrap(_initializeAppCheck, true); -export const onTokenChanged = ɵzoneWrap(_onTokenChanged, true); +export const onTokenChanged = ɵzoneWrap(_onTokenChanged, false, 1); export const setTokenAutoRefreshEnabled = ɵzoneWrap(_setTokenAutoRefreshEnabled, true); diff --git a/src/remote-config/firebase.ts b/src/remote-config/firebase.ts index 4b5540b2f..8195d43cc 100644 --- a/src/remote-config/firebase.ts +++ b/src/remote-config/firebase.ts @@ -29,6 +29,6 @@ export const getRemoteConfig = ɵzoneWrap(_getRemoteConfig, true); export const getString = ɵzoneWrap(_getString, true); export const getValue = ɵzoneWrap(_getValue, true); export const isSupported = ɵzoneWrap(_isSupported, true); -export const onConfigUpdate = ɵzoneWrap(_onConfigUpdate, true); +export const onConfigUpdate = ɵzoneWrap(_onConfigUpdate, false, 1); export const setCustomSignals = ɵzoneWrap(_setCustomSignals, true); export const setLogLevel = ɵzoneWrap(_setLogLevel, true); diff --git a/src/zones.spec.ts b/src/zones.spec.ts new file mode 100644 index 000000000..32c4c813c --- /dev/null +++ b/src/zones.spec.ts @@ -0,0 +1,246 @@ +import { EnvironmentInjector, NgZone, PendingTasks, inject, provideZoneChangeDetection, provideZonelessChangeDetection } from '@angular/core'; +import { TestBed } from '@angular/core/testing'; +import { ɵzoneWrap } from '@angular/fire'; +import { Subject } from 'rxjs'; + +type NextHandler = (value: number) => void; + +interface FakeObserver { + next?: NextHandler | null; + error?: (error: unknown) => void; + complete?: () => void; +} + +interface FakeDataConnectObserver { + onNext?: NextHandler; +} + +interface FakeOptions { + failWith?: Error; +} + +/* Stands in for a Firebase subscribe function such as onSnapshot: accepts the handler as a function + * or as an observer object, delivers one value from a timer the way the SDK delivers from its own + * async work, and returns an unsubscribe function. The timer is created while ɵzoneWrap runs the + * call outside the Angular zone, so an unwrapped handler runs there with no injection context. */ +let lastReceived: { handler: unknown; options: unknown } | undefined; +let lastHandlerError: unknown; + +function fakeSubscribe(nextOrObserver: NextHandler | FakeObserver, options?: FakeOptions): () => void { + lastReceived = { handler: nextOrObserver, options }; + const observer: FakeObserver = typeof nextOrObserver === 'function' ? { next: nextOrObserver } : nextOrObserver; + const timer = setTimeout(() => { + try { + if (options?.failWith) { + observer.error?.(options.failWith); + return; + } + observer.next?.(1); + observer.complete?.(); + } catch (error) { + lastHandlerError = error; + } + }, 0); + return () => clearTimeout(timer); +} + +function fakeDataConnectSubscribe(observer: FakeDataConnectObserver): () => void { + const timer = setTimeout(() => observer.onNext?.(1), 0); + return () => clearTimeout(timer); +} + +const flushTimers = () => new Promise(resolve => setTimeout(resolve, 10)); + +const waitFor = async (condition: () => boolean, timeoutMs = 500) => { + const startedAt = Date.now(); + while (!condition() && Date.now() - startedAt < timeoutMs) { + await flushTimers(); + } +}; + +interface HandlerReport { + values: number[]; + injectWorked: boolean | null; + inAngularZone: boolean | null; +} + +function reportingHandler(): { report: HandlerReport; next: NextHandler } { + const report: HandlerReport = { values: [], injectWorked: null, inAngularZone: null }; + const next: NextHandler = value => { + report.values.push(value); + report.inAngularZone = NgZone.isInAngularZone(); + try { + inject(EnvironmentInjector); + report.injectWorked = true; + } catch { + report.injectWorked = false; + } + }; + return { report, next }; +} + +function describeZoneWrap(zoneJs: boolean) { + /* karma loads zone.js for every spec, so the "zoneless" run only swaps NgZone for NoopNgZone. + * The Zone-undefined paths in zones.ts are not covered here. */ + const mode = zoneJs ? 'with zone.js change detection' : 'with NoopNgZone (zone.js still loaded)'; + + describe(`ɵzoneWrap ${mode}`, () => { + let addTask: jasmine.Spy; + /* One counter object per spec, captured by that spec's spy. The wrapped unsubscribe releases + * the task through a 0 ms timer, so a release can land after the spec that created it ends, + * and it must not count toward the next spec. */ + let releases: { count: number }; + let cleanups: (() => void)[]; + + beforeEach(() => { + // This TestBed gives a real NgZone unless told otherwise, so both modes are stated. + TestBed.configureTestingModule({ + providers: [zoneJs ? provideZoneChangeDetection() : provideZonelessChangeDetection()], + }); + const pendingTasks = TestBed.inject(PendingTasks); + const originalAdd = pendingTasks.add.bind(pendingTasks); + const thisSpecReleases = { count: 0 }; + releases = thisSpecReleases; + addTask = spyOn(pendingTasks, 'add').and.callFake(() => { + const remove = originalAdd(); + return () => { + thisSpecReleases.count++; + remove(); + }; + }); + cleanups = []; + lastReceived = undefined; + lastHandlerError = undefined; + }); + + afterEach(async () => { + // Handlers re-enter this test's injector, so none may outlive the TestBed. + cleanups.forEach(cleanup => cleanup()); + await flushTimers(); + }); + + const subscribeWrapped = (blockUntilFirst: boolean, ...callArguments: Parameters) => { + const wrapped = ɵzoneWrap(fakeSubscribe, blockUntilFirst); + const unsubscribe = TestBed.runInInjectionContext(() => wrapped(...callArguments)); + cleanups.push(unsubscribe); + return unsubscribe; + }; + + it('gives a handler passed as a function the injection context, the zone and a pending task', async () => { + const { report, next } = reportingHandler(); + subscribeWrapped(true, next); + expect(addTask).toHaveBeenCalledTimes(1); + await waitFor(() => releases.count === 1); + expect(report.values).toEqual([1]); + expect(report.injectWorked).toBeTrue(); + expect(report.inAngularZone).toBe(zoneJs); + expect(releases.count).toBe(1); + }); + + it('gives a handler passed as an observer object the same three things', async () => { + const { report, next } = reportingHandler(); + subscribeWrapped(true, { next }); + expect(addTask).toHaveBeenCalledTimes(1); + await waitFor(() => releases.count === 1); + expect(report.values).toEqual([1]); + expect(report.injectWorked).toBeTrue(); + expect(report.inAngularZone).toBe(zoneJs); + expect(releases.count).toBe(1); + }); + + it('registers no pending task for an observer object when blockUntilFirst is false', async () => { + const { report, next } = reportingHandler(); + subscribeWrapped(false, { next }); + expect(addTask).not.toHaveBeenCalled(); + await waitFor(() => report.values.length === 1); + expect(report.injectWorked).toBeTrue(); + expect(report.inAngularZone).toBe(zoneJs); + }); + + it('wraps the error and complete members of an observer object too', async () => { + let errorInjectWorked: boolean | null = null; + let completeInjectWorked: boolean | null = null; + const probeInject = () => { + try { + inject(EnvironmentInjector); + return true; + } catch { + return false; + } + }; + subscribeWrapped(true, { complete: () => { completeInjectWorked = probeInject(); } }); + subscribeWrapped(true, { error: () => { errorInjectWorked = probeInject(); } }, { failWith: new Error('boom') }); + await waitFor(() => releases.count === 2); + expect(completeInjectWorked).toBeTrue(); + expect(errorInjectWorked).toBeTrue(); + expect(releases.count).toBe(2); + }); + + it('recognizes the onNext member data-connect observers use', async () => { + const { report, next } = reportingHandler(); + const wrapped = ɵzoneWrap(fakeDataConnectSubscribe, true); + cleanups.push(TestBed.runInInjectionContext(() => wrapped({ onNext: next }))); + expect(addTask).toHaveBeenCalledTimes(1); + await waitFor(() => releases.count === 1); + expect(report.injectWorked).toBeTrue(); + expect(releases.count).toBe(1); + }); + + it('passes an options object without handler members through as the same reference', () => { + const options: FakeOptions = {}; + subscribeWrapped(true, () => undefined, options); + expect(lastReceived?.options).toBe(options); + }); + + it('hands Firebase a copy, leaving the caller\'s observer object unchanged across two calls', () => { + const next: NextHandler = () => undefined; + const observer: FakeObserver = { next }; + subscribeWrapped(true, observer); + subscribeWrapped(true, observer); + expect(observer.next).toBe(next); + expect(Object.keys(observer)).toEqual(['next']); + expect(lastReceived?.handler).not.toBe(observer); + }); + + it('keeps `this` for an observer written as a class instance', async () => { + class Counter { + count = 0; + next() { + this.count++; + } + } + const counter = new Counter(); + subscribeWrapped(true, counter); + await waitFor(() => counter.count === 1); + expect(counter.count).toBe(1); + }); + + it('delivers to an RxJS Subject passed as the observer', async () => { + const subject = new Subject(); + const received: number[] = []; + subject.subscribe(value => received.push(value)); + subscribeWrapped(true, subject); + await waitFor(() => received.length === 1); + expect(received).toEqual([1]); + }); + + it('passes a null next member through untouched', async () => { + let errorSeen: unknown; + subscribeWrapped(true, { next: null, error: error => { errorSeen = error; } }, { failWith: new Error('boom') }); + const handler = lastReceived?.handler; + expect(handler && typeof handler === 'object' && Reflect.get(handler, 'next')).toBeNull(); + await waitFor(() => errorSeen !== undefined); + expect(errorSeen).toEqual(new Error('boom')); + }); + + it('still releases the pending task when next throws, and lets the error reach the SDK', async () => { + subscribeWrapped(true, { next: () => { throw new Error('handler failed'); } }); + await waitFor(() => releases.count === 1); + expect(releases.count).toBe(1); + expect(lastHandlerError).toEqual(new Error('handler failed')); + }); + }); +} + +describeZoneWrap(true); +describeZoneWrap(false); diff --git a/src/zones.ts b/src/zones.ts index 62e97d2c7..6df634865 100644 --- a/src/zones.ts +++ b/src/zones.ts @@ -121,6 +121,29 @@ const zoneWrapFn = ( }; }; +/* Firebase also takes a handler as an object, and data-connect's subscribe spells its members onNext, onErr, onComplete. */ +const observerKeys = ['next', 'error', 'complete', 'onNext', 'onErr', 'onComplete']; + +const isObserverLike = (value: unknown): value is object => + typeof value === 'object' && value !== null && + observerKeys.some(key => typeof Reflect.get(value, key) === 'function'); + +/* A copy is handed to Firebase so the caller can reuse the same observer for several calls. */ +const zoneWrapObserver = ( + observer: object, + taskDone: VoidFunction | undefined, + injector: EnvironmentInjector, +): object => { + const wrapped: Record = { ...observer }; + for (const key of observerKeys) { + const member = Reflect.get(observer, key); + if (typeof member === 'function') { + wrapped[key] = zoneWrapFn(member.bind(observer), taskDone, injector); + } + } + return wrapped; +}; + export const ɵzoneWrap = (it: T, blockUntilFirst: boolean, logLevel?: LogLevel): T => { logLevel ||= blockUntilFirst ? LogLevel.WARN : LogLevel.VERBOSE; // function() is needed for the arguments object @@ -138,16 +161,21 @@ export const ɵzoneWrap = (it: T, blockUntilFirst: boolean, logLevel warnOutsideInjectionContext(it, logLevel); return (it as any).apply(this, _arguments); } - // if this is a callback function, e.g, onSnapshot, we should create a pending task and complete it - // only once one of the callback functions is tripped. + /* if this is a callback, a function or an observer object, e.g. onSnapshot, we should create a + * pending task and complete it only once one of the callback functions is tripped. */ for (let i = 0; i < arguments.length; i++) { - if (typeof _arguments[i] === 'function') { - if (blockUntilFirst) { - taskDone ||= run(() => pendingTasks.add()); - } - // TODO create a microtask to track callback functions - _arguments[i] = zoneWrapFn(_arguments[i], taskDone, injector); + const argument = _arguments[i]; + const isFunction = typeof argument === 'function'; + if (!isFunction && !isObserverLike(argument)) { + continue; + } + if (blockUntilFirst) { + taskDone ||= run(() => pendingTasks.add()); } + // TODO create a microtask to track callback functions + _arguments[i] = isFunction + ? zoneWrapFn(argument, taskDone, injector) + : zoneWrapObserver(argument, taskDone, injector); } const ret = runOutsideAngular(() => (it as any).apply(this, _arguments)); if (!blockUntilFirst) { diff --git a/tools/build.ts b/tools/build.ts index 36f11be9e..d5f363e3c 100644 --- a/tools/build.ts +++ b/tools/build.ts @@ -164,6 +164,8 @@ ${exportedZoneWrappedFns} reexport('app', 'firebase', 'firebase/app', tsKeys()), reexport('app-check', 'firebase', 'firebase/app-check', tsKeys(), { getLimitedUseToken: { logLevel: LogLevel.VERBOSE }, + // The callback runs only once a token is obtained, which a first visit may never request: `blockUntilFirst: false` otherwise `ApplicationRef.isStable` may never become `true`. + onTokenChanged: { blockUntilFirst: false, logLevel: LogLevel.WARN }, }), reexport('auth', 'rxfire', 'rxfire/auth', tsKeys()), reexport('auth', 'firebase', 'firebase/auth', tsKeys(), { @@ -278,7 +280,10 @@ ${exportedZoneWrappedFns} getBoolean: { exportName: 'getBooleanChanges' }, getAll: { exportName: 'getAllChanges' }, }), - reexport('remote-config', 'firebase', 'firebase/remote-config', tsKeys()), + reexport('remote-config', 'firebase', 'firebase/remote-config', tsKeys(), { + // The observer runs only when a fetched template differs from the active one, which a normal load never produces. + onConfigUpdate: { blockUntilFirst: false, logLevel: LogLevel.WARN }, + }), reexport('storage', 'rxfire', 'rxfire/storage', tsKeys(), { getDownloadURL: null, getMetadata: null, From 0a0bd6419b5d6eb4b5be63d93926771f7e6a09ba Mon Sep 17 00:00:00 2001 From: Armando Navarro Date: Fri, 30 Oct 2026 07:36:40 -0700 Subject: [PATCH 2/3] fix(core): let a wrapped promise settle after its injector is destroyed When a wrapped call returns a Promise, the wrapper resolved its own promise through runInInjectionContext and NgZone.run. Neither reaches the caller: a .then callback runs as a later microtask after the injection context is torn down, and zone.js runs it in the zone where .then was attached, not where resolve was called. The only effect the two layers had was on teardown. runInInjectionContext throws once its injector is destroyed, so a Firebase promise settling after ApplicationRef.destroy() or a TestBed reset threw before resolve ran and the caller's promise never settled. The pending task still opens on the call and closes on settlement. --- src/zones.spec.ts | 11 +++++++++++ src/zones.ts | 5 +---- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/zones.spec.ts b/src/zones.spec.ts index 32c4c813c..cf313d399 100644 --- a/src/zones.spec.ts +++ b/src/zones.spec.ts @@ -49,6 +49,10 @@ function fakeDataConnectSubscribe(observer: FakeDataConnectObserver): () => void return () => clearTimeout(timer); } +function fakeAsyncCall(): Promise { + return new Promise(resolve => setTimeout(() => resolve('done'), 20)); +} + const flushTimers = () => new Promise(resolve => setTimeout(resolve, 10)); const waitFor = async (condition: () => boolean, timeoutMs = 500) => { @@ -239,6 +243,13 @@ function describeZoneWrap(zoneJs: boolean) { expect(releases.count).toBe(1); expect(lastHandlerError).toEqual(new Error('handler failed')); }); + + it('settles a wrapped promise even when it resolves after the injector is destroyed', async () => { + const wrapped = ɵzoneWrap(fakeAsyncCall, true); + const promise = TestBed.runInInjectionContext(() => wrapped()); + TestBed.resetTestingModule(); + await expectAsync(promise).toBeResolvedTo('done'); + }); }); } diff --git a/src/zones.ts b/src/zones.ts index 6df634865..b17e06923 100644 --- a/src/zones.ts +++ b/src/zones.ts @@ -200,10 +200,7 @@ export const ɵzoneWrap = (it: T, blockUntilFirst: boolean, logLevel () => { const removeTask = pendingTasks.add(); return new Promise((resolve, reject) => { - ret.then( - (it) => runInInjectionContext(injector, () => run(() => resolve(it))), - (reason) => runInInjectionContext(injector, () => run(() => reject(reason))) - ).finally(removeTask); + ret.then(resolve, reject).finally(removeTask); }); }); } else if (typeof ret === 'function' && taskDone) { From e16825a5071b7ae948d2a9208ee666b2ecd35f10 Mon Sep 17 00:00:00 2001 From: Armando Navarro Date: Wed, 4 Nov 2026 16:09:19 -0800 Subject: [PATCH 3/3] fix(core): wrap observer objects only for functions that take one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit treated any object argument with a next, error or complete method as an observer. A model class with a complete() method, saved through setDoc with a converter, was replaced by a plain copy that lost its prototype, and its pending task was never released. tools/build.ts now reads Firebase's type declarations with the TypeScript compiler API and passes a fourth ɵzoneWrap argument, true, only to the 11 exports with an observer-shaped parameter. Every other function receives its object arguments untouched. A handler Firebase calls after the app is destroyed now runs directly, because entering a destroyed injector throws NG0205. Handlers passed as functions already hit this on main. --- src/app-check/firebase.ts | 2 +- src/auth/firebase.ts | 4 +- src/data-connect/firebase.ts | 2 +- src/firestore/firebase.ts | 6 +-- src/firestore/firestore.spec.ts | 47 ++++++++++++++++++- src/messaging/firebase.ts | 6 +-- src/remote-config/firebase.ts | 2 +- src/zones.spec.ts | 62 +++++++++++++++++++++++-- src/zones.ts | 17 +++++-- tools/build.ts | 81 ++++++++++++++++++++++++++++++++- 10 files changed, 206 insertions(+), 23 deletions(-) diff --git a/src/app-check/firebase.ts b/src/app-check/firebase.ts index e7928dafb..452d95875 100644 --- a/src/app-check/firebase.ts +++ b/src/app-check/firebase.ts @@ -12,5 +12,5 @@ import { export const getLimitedUseToken = ɵzoneWrap(_getLimitedUseToken, true, 2); export const getToken = ɵzoneWrap(_getToken, true); export const initializeAppCheck = ɵzoneWrap(_initializeAppCheck, true); -export const onTokenChanged = ɵzoneWrap(_onTokenChanged, false, 1); +export const onTokenChanged = ɵzoneWrap(_onTokenChanged, false, 1, true); export const setTokenAutoRefreshEnabled = ɵzoneWrap(_setTokenAutoRefreshEnabled, true); diff --git a/src/auth/firebase.ts b/src/auth/firebase.ts index 5d359e54d..ff50c99fc 100644 --- a/src/auth/firebase.ts +++ b/src/auth/firebase.ts @@ -78,8 +78,8 @@ export const linkWithCredential = ɵzoneWrap(_linkWithCredential, true, 2); export const linkWithPhoneNumber = ɵzoneWrap(_linkWithPhoneNumber, true, 2); export const linkWithPopup = ɵzoneWrap(_linkWithPopup, true, 2); export const linkWithRedirect = ɵzoneWrap(_linkWithRedirect, true, 2); -export const onAuthStateChanged = ɵzoneWrap(_onAuthStateChanged, true); -export const onIdTokenChanged = ɵzoneWrap(_onIdTokenChanged, true); +export const onAuthStateChanged = ɵzoneWrap(_onAuthStateChanged, true, undefined, true); +export const onIdTokenChanged = ɵzoneWrap(_onIdTokenChanged, true, undefined, true); export const parseActionCodeURL = ɵzoneWrap(_parseActionCodeURL, true); export const reauthenticateWithCredential = ɵzoneWrap(_reauthenticateWithCredential, true, 2); export const reauthenticateWithPhoneNumber = ɵzoneWrap(_reauthenticateWithPhoneNumber, true, 2); diff --git a/src/data-connect/firebase.ts b/src/data-connect/firebase.ts index 2597117d8..5d21cf144 100644 --- a/src/data-connect/firebase.ts +++ b/src/data-connect/firebase.ts @@ -21,6 +21,6 @@ export const getDataConnect = ɵzoneWrap(_getDataConnect, true); export const mutationRef = ɵzoneWrap(_mutationRef, true, 2); export const queryRef = ɵzoneWrap(_queryRef, true, 2); export const setLogLevel = ɵzoneWrap(_setLogLevel, true); -export const subscribe = ɵzoneWrap(_subscribe, true); +export const subscribe = ɵzoneWrap(_subscribe, true, undefined, true); export const terminate = ɵzoneWrap(_terminate, true); export const toQueryRef = ɵzoneWrap(_toQueryRef, true, 2); diff --git a/src/firestore/firebase.ts b/src/firestore/firebase.ts index 7b61867fc..37b0d0526 100644 --- a/src/firestore/firebase.ts +++ b/src/firestore/firebase.ts @@ -103,9 +103,9 @@ export const limit = ɵzoneWrap(_limit, true, 2); export const limitToLast = ɵzoneWrap(_limitToLast, true, 2); export const loadBundle = ɵzoneWrap(_loadBundle, true); export const namedQuery = ɵzoneWrap(_namedQuery, true, 2); -export const onSnapshot = ɵzoneWrap(_onSnapshot, true); -export const onSnapshotResume = ɵzoneWrap(_onSnapshotResume, true); -export const onSnapshotsInSync = ɵzoneWrap(_onSnapshotsInSync, true); +export const onSnapshot = ɵzoneWrap(_onSnapshot, true, undefined, true); +export const onSnapshotResume = ɵzoneWrap(_onSnapshotResume, true, undefined, true); +export const onSnapshotsInSync = ɵzoneWrap(_onSnapshotsInSync, true, undefined, true); export const or = ɵzoneWrap(_or, true, 2); export const orderBy = ɵzoneWrap(_orderBy, true, 2); export const query = ɵzoneWrap(_query, true, 2); diff --git a/src/firestore/firestore.spec.ts b/src/firestore/firestore.spec.ts index 71d452968..d07d1d181 100644 --- a/src/firestore/firestore.spec.ts +++ b/src/firestore/firestore.spec.ts @@ -1,9 +1,32 @@ +import { PendingTasks } from '@angular/core'; import { TestBed } from '@angular/core/testing'; import { FirebaseApp, getApp, initializeApp, provideFirebaseApp } from '@angular/fire/app'; -import { Firestore, connectFirestoreEmulator, getFirestore, provideFirestore } from '@angular/fire/firestore'; +import { + Firestore, + FirestoreDataConverter, + connectFirestoreEmulator, + doc, + getDoc, + getFirestore, + provideFirestore, + setDoc, +} from '@angular/fire/firestore'; import { COMMON_CONFIG, firestoreEmulatorPort } from '../test-config'; import { rando } from '../utils'; +/* A model class with a method that shares a name with an observer member, saved through a converter. */ +class Task { + constructor(public title: string) {} + complete() { + return this.title; + } +} + +const taskConverter: FirestoreDataConverter = { + toFirestore: task => ({ title: Reflect.get(task, 'title'), wasTaskInstance: task instanceof Task }), + fromFirestore: snapshot => new Task(String(snapshot.get('title'))), +}; + describe('Firestore', () => { let app: FirebaseApp; let firestore: Firestore; @@ -34,6 +57,28 @@ describe('Firestore', () => { expect(firestore.app).toEqual(app); }); + it('setDoc hands a converter the model instance itself and releases its pending task', async () => { + const pendingTasks = TestBed.inject(PendingTasks); + const originalAdd = pendingTasks.add.bind(pendingTasks); + let added = 0; + let released = 0; + spyOn(pendingTasks, 'add').and.callFake(() => { + added++; + const remove = originalAdd(); + return () => { + released++; + remove(); + }; + }); + const ref = TestBed.runInInjectionContext(() => doc(firestore, `tasks/${rando()}`)).withConverter(taskConverter); + await TestBed.runInInjectionContext(() => setDoc(ref, new Task('write the spec'))); + await new Promise(resolve => setTimeout(resolve, 10)); + expect(added).toBe(1); + expect(released).toBe(1); + const stored = await TestBed.runInInjectionContext(() => getDoc(doc(firestore, ref.path))); + expect(stored.get('wasTaskInstance')).toBeTrue(); + }); + }); }); diff --git a/src/messaging/firebase.ts b/src/messaging/firebase.ts index b65f2d895..4c1a29760 100644 --- a/src/messaging/firebase.ts +++ b/src/messaging/firebase.ts @@ -17,8 +17,8 @@ export const deleteToken = ɵzoneWrap(_deleteToken, true, 2); export const getMessaging = ɵzoneWrap(_getMessaging, true); export const getToken = ɵzoneWrap(_getToken, true); export const isSupported = ɵzoneWrap(_isSupported, false); -export const onMessage = ɵzoneWrap(_onMessage, false); -export const onRegistered = ɵzoneWrap(_onRegistered, false); -export const onUnregistered = ɵzoneWrap(_onUnregistered, false); +export const onMessage = ɵzoneWrap(_onMessage, false, undefined, true); +export const onRegistered = ɵzoneWrap(_onRegistered, false, undefined, true); +export const onUnregistered = ɵzoneWrap(_onUnregistered, false, undefined, true); export const register = ɵzoneWrap(_register, true); export const unregister = ɵzoneWrap(_unregister, true, 2); diff --git a/src/remote-config/firebase.ts b/src/remote-config/firebase.ts index 8195d43cc..bdcdb5855 100644 --- a/src/remote-config/firebase.ts +++ b/src/remote-config/firebase.ts @@ -29,6 +29,6 @@ export const getRemoteConfig = ɵzoneWrap(_getRemoteConfig, true); export const getString = ɵzoneWrap(_getString, true); export const getValue = ɵzoneWrap(_getValue, true); export const isSupported = ɵzoneWrap(_isSupported, true); -export const onConfigUpdate = ɵzoneWrap(_onConfigUpdate, false, 1); +export const onConfigUpdate = ɵzoneWrap(_onConfigUpdate, false, 1, true); export const setCustomSignals = ɵzoneWrap(_setCustomSignals, true); export const setLogLevel = ɵzoneWrap(_setLogLevel, true); diff --git a/src/zones.spec.ts b/src/zones.spec.ts index cf313d399..c082aed92 100644 --- a/src/zones.spec.ts +++ b/src/zones.spec.ts @@ -19,13 +19,13 @@ interface FakeOptions { failWith?: Error; } +let lastReceived: { handler: unknown; options: unknown } | undefined; +let lastHandlerError: unknown; + /* Stands in for a Firebase subscribe function such as onSnapshot: accepts the handler as a function * or as an observer object, delivers one value from a timer the way the SDK delivers from its own * async work, and returns an unsubscribe function. The timer is created while ɵzoneWrap runs the * call outside the Angular zone, so an unwrapped handler runs there with no injection context. */ -let lastReceived: { handler: unknown; options: unknown } | undefined; -let lastHandlerError: unknown; - function fakeSubscribe(nextOrObserver: NextHandler | FakeObserver, options?: FakeOptions): () => void { lastReceived = { handler: nextOrObserver, options }; const observer: FakeObserver = typeof nextOrObserver === 'function' ? { next: nextOrObserver } : nextOrObserver; @@ -53,6 +53,22 @@ function fakeAsyncCall(): Promise { return new Promise(resolve => setTimeout(() => resolve('done'), 20)); } +/* A model class like one an app saves through a Firestore converter, with a method that happens to + * share a name with an observer member. */ +class Task { + constructor(public title: string) {} + complete() { + return this.title; + } +} + +// Stands in for a Firebase data call such as setDoc: takes a data object and returns a Promise. +let lastSaved: unknown; +function fakeSave(data: object): Promise { + lastSaved = data; + return new Promise(resolve => setTimeout(resolve, 0)); +} + const flushTimers = () => new Promise(resolve => setTimeout(resolve, 10)); const waitFor = async (condition: () => boolean, timeoutMs = 500) => { @@ -115,6 +131,7 @@ function describeZoneWrap(zoneJs: boolean) { cleanups = []; lastReceived = undefined; lastHandlerError = undefined; + lastSaved = undefined; }); afterEach(async () => { @@ -124,12 +141,17 @@ function describeZoneWrap(zoneJs: boolean) { }); const subscribeWrapped = (blockUntilFirst: boolean, ...callArguments: Parameters) => { - const wrapped = ɵzoneWrap(fakeSubscribe, blockUntilFirst); + const wrapped = ɵzoneWrap(fakeSubscribe, blockUntilFirst, undefined, true); const unsubscribe = TestBed.runInInjectionContext(() => wrapped(...callArguments)); cleanups.push(unsubscribe); return unsubscribe; }; + const subscribeWrappedWithoutObserver = (blockUntilFirst: boolean, ...callArguments: Parameters) => { + const wrapped = ɵzoneWrap(fakeSubscribe, blockUntilFirst); + cleanups.push(TestBed.runInInjectionContext(() => wrapped(...callArguments))); + }; + it('gives a handler passed as a function the injection context, the zone and a pending task', async () => { const { report, next } = reportingHandler(); subscribeWrapped(true, next); @@ -182,7 +204,7 @@ function describeZoneWrap(zoneJs: boolean) { it('recognizes the onNext member data-connect observers use', async () => { const { report, next } = reportingHandler(); - const wrapped = ɵzoneWrap(fakeDataConnectSubscribe, true); + const wrapped = ɵzoneWrap(fakeDataConnectSubscribe, true, undefined, true); cleanups.push(TestBed.runInInjectionContext(() => wrapped({ onNext: next }))); expect(addTask).toHaveBeenCalledTimes(1); await waitFor(() => releases.count === 1); @@ -244,6 +266,36 @@ function describeZoneWrap(zoneJs: boolean) { expect(lastHandlerError).toEqual(new Error('handler failed')); }); + it('still calls a handler that fires after the injector is destroyed, in both forms', async () => { + const received: unknown[] = []; + subscribeWrapped(true, value => received.push(value)); + subscribeWrapped(true, { next: value => received.push(value) }); + TestBed.resetTestingModule(); + await flushTimers(); + expect(received).toEqual([1, 1]); + expect(lastHandlerError).toBeUndefined(); + }); + + it('leaves a data object with an observer-named method alone when the call takes no observer', async () => { + const wrapped = ɵzoneWrap(fakeSave, true); + const task = new Task('write the spec'); + await TestBed.runInInjectionContext(() => wrapped(task)); + await flushTimers(); + expect(lastSaved).toBe(task); + expect(addTask).toHaveBeenCalledTimes(1); + expect(releases.count).toBe(1); + }); + + it('does not treat an observer object as one when the call takes no observer', async () => { + const { report, next } = reportingHandler(); + const observer: FakeObserver = { next }; + subscribeWrappedWithoutObserver(true, observer); + expect(addTask).not.toHaveBeenCalled(); + expect(lastReceived?.handler).toBe(observer); + await waitFor(() => report.values.length === 1); + expect(report.injectWorked).toBeFalse(); + }); + it('settles a wrapped promise even when it resolves after the injector is destroyed', async () => { const wrapped = ɵzoneWrap(fakeAsyncCall, true); const promise = TestBed.runInInjectionContext(() => wrapped()); diff --git a/src/zones.ts b/src/zones.ts index b17e06923..c606d477f 100644 --- a/src/zones.ts +++ b/src/zones.ts @@ -117,18 +117,22 @@ const zoneWrapFn = ( if (taskDone) { setTimeout(taskDone, 0); } + // Firebase can call a handler after the app is destroyed, and entering a destroyed injector throws NG0205. + if (injector.destroyed) { + return it.apply(this, args); + } return runInInjectionContext(injector, () => run(() => it.apply(this, args))); }; }; -/* Firebase also takes a handler as an object, and data-connect's subscribe spells its members onNext, onErr, onComplete. */ +/** Firebase also takes a handler as an object, and data-connect's subscribe spells its members onNext, onErr, onComplete. */ const observerKeys = ['next', 'error', 'complete', 'onNext', 'onErr', 'onComplete']; const isObserverLike = (value: unknown): value is object => typeof value === 'object' && value !== null && observerKeys.some(key => typeof Reflect.get(value, key) === 'function'); -/* A copy is handed to Firebase so the caller can reuse the same observer for several calls. */ +/** A copy is handed to Firebase so the caller can reuse the same observer for several calls. */ const zoneWrapObserver = ( observer: object, taskDone: VoidFunction | undefined, @@ -144,7 +148,12 @@ const zoneWrapObserver = ( return wrapped; }; -export const ɵzoneWrap = (it: T, blockUntilFirst: boolean, logLevel?: LogLevel): T => { +/** + * `acceptsObserver` is set by tools/build.ts only for functions whose declared parameters include an + * observer, so a data object that happens to have a next, error or complete method reaches every + * other function untouched. + */ +export const ɵzoneWrap = (it: T, blockUntilFirst: boolean, logLevel?: LogLevel, acceptsObserver = false): T => { logLevel ||= blockUntilFirst ? LogLevel.WARN : LogLevel.VERBOSE; // function() is needed for the arguments object return function () { @@ -166,7 +175,7 @@ export const ɵzoneWrap = (it: T, blockUntilFirst: boolean, logLevel for (let i = 0; i < arguments.length; i++) { const argument = _arguments[i]; const isFunction = typeof argument === 'function'; - if (!isFunction && !isObserverLike(argument)) { + if (!isFunction && !(acceptsObserver && isObserverLike(argument))) { continue; } if (blockUntilFirst) { diff --git a/tools/build.ts b/tools/build.ts index d5f363e3c..be305c8f6 100644 --- a/tools/build.ts +++ b/tools/build.ts @@ -4,6 +4,7 @@ import { join, resolve, sep } from 'path'; import { keys as tsKeys } from 'ts-transformer-keys'; import * as esbuild from "esbuild"; import type { ReplaceResult } from 'replace-in-file'; +import * as ts from 'typescript'; enum LogLevel { "WARN" = 1, @@ -74,6 +75,69 @@ const overridesFirebaseDoesNotDeclare: Record = {}; type Overrides = Record; +/* Must match observerKeys in src/zones.ts, which applies the same test at runtime. */ +const observerMemberNames = ['next', 'error', 'complete', 'onNext', 'onErr', 'onComplete']; + +const checkerOptions: ts.CompilerOptions = { + target: ts.ScriptTarget.ES2022, + module: ts.ModuleKind.ESNext, + moduleResolution: ts.ModuleResolutionKind.Bundler, + strict: true, + skipLibCheck: true, + lib: ['lib.es2022.d.ts', 'lib.dom.d.ts'], + types: [], +}; + +/** The members of a union other than undefined, null and void, so an optional type is judged by what it can hold. */ +const withoutEmptyTypes = (type: ts.Type): ts.Type[] => + (type.isUnion() ? type.types : [type]) + .filter(part => !(part.flags & (ts.TypeFlags.Undefined | ts.TypeFlags.Null | ts.TypeFlags.Void))); + +const isCallable = (type: ts.Type) => withoutEmptyTypes(type).some(part => part.getCallSignatures().length > 0); + +/** An object type, not a function, with at least one observer member that is a function. */ +const isObserverShaped = (checker: ts.TypeChecker, type: ts.Type) => + withoutEmptyTypes(type).some(part => !isCallable(part) && observerMemberNames.some(memberName => { + const member = part.getProperty(memberName); + return member !== undefined && isCallable(checker.getTypeOfSymbol(member)); + })); + +interface CheckedModule { + checker: ts.TypeChecker; + moduleSymbol: ts.Symbol; +} + +/** Type-checks an in-memory file that imports `path`, to read that entry point's declarations. */ +function checkModule(path: string): CheckedModule { + // TypeScript reports file names with forward slashes, also on Windows. + const fileName = join(process.cwd(), '__observer-check__.ts').split(sep).join('/'); + const source = ts.createSourceFile(fileName, `import * as checked from '${path}';\n`, ts.ScriptTarget.ES2022); + const host = ts.createCompilerHost(checkerOptions); + const getSourceFile = host.getSourceFile.bind(host); + const fileExists = host.fileExists.bind(host); + host.getSourceFile = (requested, languageVersion) => + requested === fileName ? source : getSourceFile(requested, languageVersion); + host.fileExists = requested => requested === fileName || fileExists(requested); + const checker = ts.createProgram([fileName], checkerOptions, host).getTypeChecker(); + const importDeclaration = source.statements.find(ts.isImportDeclaration); + const moduleSymbol = importDeclaration && checker.getSymbolAtLocation(importDeclaration.moduleSpecifier); + if (!moduleSymbol) { + throw new Error(`Could not read the type declarations of ${path} to find its observer parameters.`); + } + return { checker, moduleSymbol }; +} + +/** The exports of `path` with a parameter shaped like an observer, read from its type declarations. */ +function exportsTakingAnObserver(path: string): Set { + const { checker, moduleSymbol } = checkModule(path); + const takesAnObserver = (exported: ts.Symbol) => { + const declared = exported.flags & ts.SymbolFlags.Alias ? checker.getAliasedSymbol(exported) : exported; + return checker.getTypeOfSymbol(declared).getCallSignatures().some(signature => + signature.getParameters().some(parameter => isObserverShaped(checker, checker.getTypeOfSymbol(parameter)))); + }; + return new Set(checker.getExportsOfModule(moduleSymbol).filter(takesAnObserver).map(exported => exported.name)); +} + function zoneWrapExports() { /* One overrides object can be applied to several entry points, so the keys it may legally name * are the union of their export lists. firestoreOverrides covers both firestore and @@ -111,8 +175,21 @@ function zoneWrapExports() { const rawExportedFns = rawExport.map(([importName, exportName]) => `${importName}${exportName === importName ? '' : `as ${exportName}`}`).join(',\n '); const overriddenFns = overridden.join(',\n '); + /* The ɵzoneWrap arguments after the wrapped function, in order: blockUntilFirst, logLevel, + * acceptsObserver. The arguments are positional, so a function that takes an observer gets + * logLevel written out, as `undefined` when it has no override, to reach the observer flag. */ + const observerTakingExports = exportsTakingAnObserver(path); + const zoneWrapArguments = (importName: string) => { + const logLevel = overrides[importName]?.logLevel; + const acceptsObserver = observerTakingExports.has(importName); + return [ + `${overrides[importName]?.blockUntilFirst ?? true}`, + ...(logLevel || acceptsObserver ? [`${logLevel ?? 'undefined'}`] : []), + ...(acceptsObserver ? ['true'] : []), + ].join(', '); + }; const exportedZoneWrappedFns = zoneWrapped.map(([importName, exportName]) => - `export const ${exportName} = ɵzoneWrap(_${importName}, ${overrides[importName]?.blockUntilFirst ?? true}${overrides[importName]?.logLevel ? `, ${overrides[importName].logLevel}` : ""});`) + `export const ${exportName} = ɵzoneWrap(_${importName}, ${zoneWrapArguments(importName)});`) .join('\n'); const filePath = join(process.cwd(), 'src', `${module}/${name}.ts`); // TODO(davideast): Create a builder pattern for this file for readability @@ -281,7 +358,7 @@ ${exportedZoneWrappedFns} getAll: { exportName: 'getAllChanges' }, }), reexport('remote-config', 'firebase', 'firebase/remote-config', tsKeys(), { - // The observer runs only when a fetched template differs from the active one, which a normal load never produces. + // The observer runs only when a fetched template differs from the active one, so it may never fire. onConfigUpdate: { blockUntilFirst: false, logLevel: LogLevel.WARN }, }), reexport('storage', 'rxfire', 'rxfire/storage', tsKeys(), {