GeezerVison is showing github.com — fix(core): wrap observer-object handlers like functions, and let a wrapped promise settle after teardown by armando-navarro · Pull Request #3829 · angular/angularfire
Conversation
Fixes #3828
ɵzoneWrap now wraps a handler passed as an observer object the way it wraps a handler passed as a function, for the 11 functions whose declared parameters include an observer. Firebase receives a copy of the object whose next, error, complete, onNext, onErr and onComplete members are wrapped and bound to the caller's object.
tools/build.ts finds those 11 at build time by reading Firebase's type declarations with the TypeScript compiler API, and passes them a fourth ɵzoneWrap argument, true. Every other wrapped function passes its object arguments through untouched.
onConfigUpdate (remote-config) gets blockUntilFirst: false, because wrapping its observer at true would hold the application unstable until a config update a normal load may never produce.
- The promise branch resolves and rejects directly, dropping the
runInInjectionContext and NgZone.run layers that never reached the caller and threw once the injector was destroyed.
- A handler Firebase calls after its injector is destroyed now runs instead of throwing
NG0205, in both forms.
Checklist
- Issue number for this PR:
ɵzoneWrap ignores a handler passed as an observer object, and holds a wrapped promise forever once its injector is destroyed #3828
- Docs included?: no. One sentence in
docs/zones.md, saying the re-entry holds for a handler passed as an object too, goes in the rc.1 docs PR, docs: document the 22.0.0-rc.1 changes #3835.
- Test units included?: yes.
src/zones.spec.ts has fifteen cases, each run under provideZoneChangeDetection() and under provideZonelessChangeDetection(). src/firestore/firestore.spec.ts has one more, against the real setDoc and the emulator.
- Breaking change?: no
- In a clean directory,
yarn install, yarn test run successfully?: yes, with npm: npm ci, npm run build, npm test (which runs build:jasmine, test:node-esm and test:chrome-headless), plus npm run test:node and npm run lint.
Changes
Before this change the argument loop in ɵzoneWrap tested typeof _arguments[i] === 'function' and nothing else, so an observer object reached the SDK untouched. The issue lists the eleven affected functions and what each form lost.
The first commit:
- Adds
isObserverLike, true for a non-null object with a function under any of the six member names, and zoneWrapObserver, which returns a copy of the object with those members replaced by the same wrapper a function argument gets.
- The copy keeps the caller's object untouched, so one observer can be reused across calls, and
.bind(observer) keeps this for a class instance or an RxJS Subject.
- Adds
onConfigUpdate: { blockUntilFirst: false, logLevel: LogLevel.WARN } to the remote-config block of tools/build.ts.
The second commit:
- Replaces
ret.then((it) => runInInjectionContext(injector, () => run(() => resolve(it))), ...) with ret.then(resolve, reject).
- The two layers never reached the caller's continuation, and their
assertNotDestroyed check is what hung a promise settling after teardown (the issue has the details).
- The pending task still opens on the call and closes on settlement.
The third commit:
- Narrows the first commit, which treated any object argument with one of those members as an observer. A model class with a
complete() method, saved through setDoc with a converter, reached the converter as a plain copy without its prototype, and the call's pending task was never released.
- Adds
exportsTakingAnObserver to tools/build.ts. It type-checks an in-memory file that imports each Firebase entry point, and keeps the exports with a parameter whose type is an object with a function-valued observer member.
- Those 11 exports are generated as
ɵzoneWrap(fn, blockUntilFirst, logLevel, true).
ɵzoneWrap looks for an observer object only when that fourth argument is true.
- Makes
zoneWrapFn call the handler directly when injector.destroyed is true. Before, a handler Firebase called after ApplicationRef.destroy() or a TestBed reset threw NG0205, which already happened on main for handlers passed as functions.
The merge of main:
What callers can notice
onSnapshot(ref, { next }) and the other object-form calls now hold server rendering until the first value, run their members inside the Angular zone, and can call inject() inside them, exactly like the function form.
onConfigUpdate, which never held the application unstable (its only form was never wrapped), does not start to now that its observer is.
- Every other wrapped function,
setDoc included, receives its object arguments exactly as before this PR.
- A wrapped promise that settles after its injector is destroyed now settles. Nothing else about the promise branch changes: the continuation's zone and the behavior of
inject() inside it are the same as before.
- A handler that runs after its injector is destroyed runs without the Angular zone and the injection context, instead of throwing
NG0205.
- The SDK receives a copy of the observer rather than the caller's object. None of the eleven SDK functions compares the object's identity: each reads or binds its members, or keeps the object it received and removes that same one on unsubscribe.
Verification
- Removing the
injector.destroyed check from zoneWrapFn fails the new teardown spec in both modes with NG0205: Injector has already been destroyed.
- Removing the fourth-argument check from
ɵzoneWrap fails five specs: the two data-object cases in both modes, and the Firestore setDoc converter spec.
- Full karma suite in headless Chrome on the merged branch: 169 of 169 pass.
npm run test:node and npm run test:node-esm: 401 specs, 0 failures each. npm run lint passes.
- With firebase 13.1.0 installed in place of 12.19.0, the build flags the same 11 exports.
- Measured in Chrome on a zone.js Angular 22.2 app and a zoneless one with the built tarball installed, against the Firestore emulator, through the generated
@angular/fire/firestore and @angular/fire/remote-config entry points: the object form matches the function form on pending task, zone and inject() in both apps, and onConfigUpdate registers no task. This was rerun with the build-time flag in place, before the NG0205 change and the merge.
- Same apps, the promise branch:
getDoc registers and releases its task and resolves with the document, a rejecting wrapped call releases its task and the rejection reaches the caller, the caller's continuation sees the same zone and the same inject() failure as before, and a wrapped call made inside a child EnvironmentInjector that is destroyed before the promise settles resolves.
Backport
Non-breaking. src/zones.ts on 21.1.x is identical to this branch's starting point. A cherry-pick PR against 21.1.x follows at a later date, after a run on an Angular 21 app.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving. A fresh build regenerates both files byte-identical to the PR, Chrome headless passes 140/140, and rebuilding with main's src/zones.ts fails exactly the 14 zones.spec.ts cases the body names.
Non-blocking nits:
- The PR title is
fix: but both commits use fix(core):, which is what the squash should carry. Worth retitling before merge.
src/zones.ts adds /* */ comments among the existing // ones. The content is worth keeping; only the style differs.
isObserverLike reads members with Reflect.get, so it also matches a next, error or complete inherited through the prototype, and any wrapped call's object argument that has one gets copied. I don't know of a Firebase argument type like that, but I haven't audited every wrapped signature, so this is a question rather than a finding.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
armando-navarro
changed the title
fix: wrap observer-object handlers like functions, and let a wrapped promise settle after teardown
fix(core): wrap observer-object handlers like functions, and let a wrapped promise settle after teardown
Oct 30, 2026
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.
# Conflicts:
# src/app-check/firebase.ts
# src/remote-config/firebase.ts
# src/zones.spec.ts
# tools/build.ts
|
Thanks for the review, @tyler-reitz. Your third point turned up a bug this PR introduced, so the branch has changed since your approval. Can you take another look?
Title
Retitled to fix(core):.
Comment style
The /** */ and /* */ comments are intentional: /** */ above module-level declarations, and /* */ for a comment spanning several lines inside a function.
isObserverLike and object arguments
Even though you approved the PR, I felt this was significant enough to take care of properly and request another review. A model class with a complete() method, saved through setDoc with a converter, was treated as an observer:
- the converter received a plain copy that was no longer an instance of the class
- the call's pending task was never released
e16825a fixes it at build time:
tools/build.ts reads Firebase's type declarations and marks only the 11 functions whose parameters include an observer.
ɵzoneWrap looks for an observer object only in those 11.
- Every other wrapped function,
setDoc included, gets its object arguments untouched.
- A Firestore spec against the real
setDoc and the emulator covers the converter case.
Also new since your approval
The description has the details.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-approving on 731b063 after the third commit and the merge.
Checked locally: a build reproduces the generated firebase.ts files with no drift, karma passes 169/169, and removing the acceptsObserver guard and the injector.destroyed check fails the 7 specs the description says it should.
Non-blocking: the six observer member names live in both src/zones.ts and tools/build.ts, held together only by the "must match" comment. Fine to leave; worth a shared constant if either list ever changes.
Fixes #3828 will close the issue on merge, so the 21.1.x backport needs tracking somewhere else.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
armando-navarro
added a commit
to armando-navarro/angularfire
that referenced
this pull request
Nov 5, 2026
…rotask
angular#3829's destroyed-injector early return now sits inside the try, so the
finally removes the pending task on that path too. The argument loop is
main's, without the TODO this branch deleted. angular#3829's spec comment about
a 0 ms timer now gives the reason the per-spec counter still holds.
# Conflicts:
# src/zones.spec.ts
# src/zones.ts
armando-navarro
added a commit
to armando-navarro/angularfire
that referenced
this pull request
Nov 5, 2026
angular#3829 wraps a handler passed as an observer object, such as { next, error }, the way it wraps a function, so the injection context reaches both forms.
Refs angular#3828
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|