From 6456f327113bca163693ca10151e50ae6df3f5a3 Mon Sep 17 00:00:00 2001 From: Armando Navarro Date: Fri, 30 Oct 2026 07:33:50 -0700 Subject: [PATCH 1/2] 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/2] 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) {