GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

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

@armando-navarro

@armando-navarro armando-navarro commented Oct 30, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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.
    • Its handler runs only when a fetched template differs from the active one.
    • Adds the same entry for app-check's onTokenChanged, which fix: stop twelve wrapped calls from holding the app unstable #3827 also added on main, so after the merge only the onConfigUpdate entry is new.

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:

  • Brings in fix: stop twelve wrapped calls from holding the app unstable #3827, feat: accept firebase 13 alongside firebase 12 #3831 and feat(zones)!: deliver Observable read values as microtasks #3833.
  • The remote-config override block keeps both fix: stop twelve wrapped calls from holding the app unstable #3827's isSupported entry and this PR's onConfigUpdate entry.
  • src/zones.spec.ts, which both sides created, keeps both sets of specs.
  • The generated files come from the merged build.

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.

All reactions
armando-navarro added 2 commits October 30, 2026 07:33
@armando-navarro
fix(core): wrap a handler passed as an observer object like a function
6456f32
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.
@armando-navarro
fix(core): let a wrapped promise settle after its injector is destroyed
0a0bd64
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.
@armando-navarro
armando-navarro requested a review from tyler-reitz October 30, 2026 15:25
@armando-navarro armando-navarro added backport: 21.1.x Cherry-pick onto the 21.1.x branch for a 21.1 minor release bump: patch Size of the change: a fix or docs change that reaches users comp: app-check App Check (src/app-check). comp: core FirebaseApp DI core (src/app). comp: firestore Cloud Firestore (src/firestore). comp: remote-config Remote Config (src/remote-config). comp: zones Change detection / zone.js / zoneless. type: bug Defect: expected behavior doesn't happen. labels Oct 30, 2026
tyler-reitz
tyler-reitz approved these changes Oct 30, 2026
View reviewed changes

@tyler-reitz tyler-reitz left a comment

Copy link
Copy Markdown
Collaborator

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.

All reactions
@armando-navarro 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
armando-navarro added 2 commits November 4, 2026 16:09
@armando-navarro
fix(core): wrap observer objects only for functions that take one
e16825a
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.
@armando-navarro
Merge remote-tracking branch 'origin/main' into a52-observer-handlers
731b063
# Conflicts:
#	src/app-check/firebase.ts
#	src/remote-config/firebase.ts
#	src/zones.spec.ts
#	tools/build.ts
@armando-navarro

armando-navarro commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

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

  • e16825a also stops a handler that Firebase calls after the injector is destroyed from throwing NG0205. This already happened on main for handlers passed as functions.
  • 731b063 merges main (fix: stop twelve wrapped calls from holding the app unstable #3827, feat: accept firebase 13 alongside firebase 12 #3831 and feat(zones)!: deliver Observable read values as microtasks #3833). The only conflicts were the remote-config override block and src/zones.spec.ts, and both keep both sides.

The description has the details.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@armando-navarro
armando-navarro requested a review from tyler-reitz November 5, 2026 00:34
@armando-navarro armando-navarro added this to the 22.0.0-rc.1 milestone Nov 5, 2026
tyler-reitz
tyler-reitz approved these changes Nov 5, 2026
View reviewed changes

@tyler-reitz tyler-reitz left a comment

Copy link
Copy Markdown
Collaborator

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.

All reactions
@tyler-reitz tyler-reitz mentioned this pull request Nov 5, 2026
feat(zones)!: remove callback pending tasks right after the callback #3837
Merged
@armando-navarro
armando-navarro merged commit 916435a into angular:main Nov 5, 2026
25 checks passed

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
@armando-navarro
Merge remote-tracking branch 'origin/main' into a95-callback-task-mic…
f0b8be6
…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
@armando-navarro
docs: say a handler passed as an observer object is wrapped too
1c85fbe
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
@armando-navarro armando-navarro mentioned this pull request Nov 5, 2026
docs: document the 22.0.0-rc.1 changes #3835
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tyler-reitz tyler-reitz tyler-reitz approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

backport: 21.1.x Cherry-pick onto the 21.1.x branch for a 21.1 minor release bump: patch Size of the change: a fix or docs change that reaches users comp: app-check App Check (src/app-check). comp: core FirebaseApp DI core (src/app). comp: firestore Cloud Firestore (src/firestore). comp: remote-config Remote Config (src/remote-config). comp: zones Change detection / zone.js / zoneless. type: bug Defect: expected behavior doesn't happen.

Projects

None yet

Milestone

22.0.0-rc.1

Development

Successfully merging this pull request may close these issues.

ɵzoneWrap ignores a handler passed as an observer object, and holds a wrapped promise forever once its injector is destroyed

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@armando-navarro @tyler-reitz
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.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.