Repository navigation
fix: stop twelve wrapped calls from holding the app unstable - #3827
Conversation
ɵzoneWrap holds a pending task from the moment a wrapped call gets a callback until that callback first runs. For onLog, onTokenChanged, the Database child listeners (onChildAdded on an empty node, the other three until a later change), off, enableLogging, and the rxfire streams fromRef, stateChanges and auditTrail, nothing guarantees that first run, so ApplicationRef.isStable can stay false for good and an SSR build's route extraction times out, as angular#3748 reported for beforeAuthStateChanged. The eleven keep logLevel WARN, so only the pending task changes. isSupported in remote-config is the twelfth: angular#3613 put its blockUntilFirst: false entry in the rxfire block, which exports no isSupported, so it never applied, and angular#3759 removed the dead entry. Server renders no longer wait for onChildAdded's first batch (onValue, object and list still wait), and off(ref, type, cb) still detaches nothing, because the wrapper replaces the callback it is given. Fixes angular#3826
tyler-reitz
left a comment
There was a problem hiding this comment.
One blocking point: the fromRef override applies to every event, not only the child events the body describes.
In rxfire, object(query) is fromRef(query, ListenEvent.value), so after this change object(ref) still holds the render but the documented fromRef(ref, 'value') doesn't. A probe spec on this branch shows it: fromRef(ref(database, rando()), ListenEvent.value) registers 0 pending tasks, while object(ref(database, rando())) registers 1. On main, fromRef is generated with true, so I'd expect it to register one there (read from the generated file, not run).
Overrides match by name, so I don't think tools/build.ts can narrow this, and the tradeoff may well be right, since a child-event fromRef can hang forever. Either way, please add it to "What callers can notice" and to the docs follow-up, plus a spec pinning the 'value' case so the behavior is deliberate rather than incidental.
Non-blocking: two stray blank lines in the database override block in tools/build.ts, after enableLogging and after onChildRemoved.
Everything else checked out: a fresh build regenerates the five files byte-identical to the PR, and Chrome headless passes 132/132 locally.
fromRef takes the event to listen for, and one blockUntilFirst value cannot fit both uses: value always fires on attach, while a child event may never fire. A hand-written fromRef in overrides.ts wraps rxfire's fromRef both ways and picks by event, so fromRef(ref, ListenEvent.value) holds a server render like object(ref) and child events do not.
|
Thanks, Tyler. Rather than document the
So Specs cover every event: The PR description now says the above. The two blank lines are intentional: each marks where the comment above it stops applying, so |
tyler-reitz
left a comment
There was a problem hiding this comment.
Thanks, the per-event fromRef override resolves my blocking point. I confirmed the new specs go red when the override is forced to never wait (value spec) and always wait (all four child specs), and a fresh build regenerates the five files byte-identical. One small thing for the squash: the first commit's body still lists fromRef among the calls that no longer hold the app, so please drop it from that list in the final message.
* docs: say which wrapped calls do not hold the app unstable docs/zones.md says callback APIs hold the app unstable until an initial value, so a server render waits for data. #3827 makes a set of calls exceptions because their first callback may never come. Waiting on a callback that never comes would keep the app from ever becoming stable. List those calls in docs/zones.md, and point at onValue, object, list, listVal and fromRef with the value event as the calls that still make a server render wait. In docs/database.md, say the same under stateChanges, auditTrail and fromRef. Refs #3826 * docs: describe firebase 13 support in the v22 upgrade guide #3831 lets AngularFire 22 accept firebase 12 or 13, and the ng update migration now writes the range of the firebase major already installed. The upgrade guide still said the migration writes ^12.18.0 and quoted peer warnings that name only that range. State the supported range and what the migration writes, update the Yarn 1 and pnpm 9/10 warnings to their current wording, and add a section on moving to firebase 13: VertexAIBackend is removed and its packages declare Node 24.12 or later. Refs #3830 * docs: describe microtask delivery in the v22 upgrade guide #3833 moves every Observable read from a setInterval timer to the next microtask checkpoint. As a breaking change it needs a section in the upgrade guide: which tests now see a value they used to wait for, the setTimeout ordering flip, and the zoneless server render case for hand-written TransferState entries. Refs #3832 * docs: describe the callback pending-task change in the v22 upgrade guide #3837 removes a callback read's pending task right after the callback instead of one setTimeout(fn, 0) delay later. As a breaking change it needs a section in the upgrade guide: what moves sooner, and which tests notice. Refs #3836 * docs: say a handler passed as an observer object is wrapped too #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 #3828 * docs(compat): add a setup section in NgModule and standalone form The compat API's setup steps left docs/install-and-setup.md when #3236 rewrote it for the modular API, and nothing replaced them. docs/compat.md had no setup at all, the Realtime Database pages pointed at an "Installation guide" that no longer covered compat, and no page showed the standalone form. A standalone app with only the modular providers then fails its first compat injection with NG0201 for angularfire2.app.options (#3683). The new section on docs/compat.md gives both forms, says when the modular providers may stay, states the error the missing module produces, and lists the compat module of each product. The twelve compat pages that had an NgModule-only snippet or no setup at all now point at it. Fixes #3683 * docs: say which package managers refuse an incompatible firebase The v22 upgrade guide said an incompatible firebase is refused at install time. Only npm refuses it, and only when package.json requests the incompatible version. Yarn and pnpm install it with a warning. * docs: say which Firestore functions are not zone wrapped #3842 re-exports Firestore's query and reference builders, field values, comparators and setLogLevel without the zone wrapper. In docs/zones.md, add a paragraph listing them, change the switchMap example to a read, since building the query no longer warns, and say the Verbose log level covers only wrapped calls. Refs #3842 * docs(auth): build the server app with initializeBrowserOrServerApp The server-rendering guide's step 4 becomes the one call, the five details become three, the App Check token gets a relay section, the response gets a Cache-Control header, and the manual callback stays in a "Building the app yourself" section that step 4 points at, for apps that prefer it or whose AngularFire version has no such function. The release modes say what a throwing ngOnDestroy does to a timed release. * docs(auth): describe the sample as an example of the one-call server app The sample app now has one provideFirebaseApp with initializeBrowserOrServerApp in its shared app.config.ts (#3845), so the sentence about its split client and server files no longer holds. Refs #3843 * docs: say how to move from 22.0.0-rc.0 in the v22 upgrade guide The guide covered upgrades from 21, a 21 release candidate and a 22 canary build, but not from the first 22 release candidate. ng add saves a prerelease as an exact version, so an app on 22.0.0-rc.0 stays there until ng update @angular/fire@next moves it. That command also runs migration-v22 again, which reports "No changes made" for a firebase entry already inside the supported range. Refs #3834 * docs(auth): say which hooks stop a releaseAfterRender release The caveat on releaseAfterRender said a throwing ngOnDestroy "stops that teardown". It now says the release is a DestroyRef.onDestroy callback, that Angular runs it after every service's and component's ngOnDestroy, and that an ngOnDestroy method or an ApplicationRef.onDestroy listener that throws keeps it from running. The default release, which does not run during the injector's destroy, is unaffected either way. A new paragraph says to call initializeBrowserOrServerApp in app.config.ts rather than a route's providers: a route's child injector is not destroyed with the request, so a release registered there never runs. Refs #3844 * docs(auth): say the default release still happens from a route's providers Tyler asked on #3844 whether the default release, now tied to a root service, releases a server app built in a lazy route's providers. It does: the request's root injector is destroyed, the route's child injector is not, so only a releaseAfterRender release registered there is lost. Checked with a child environment injector that is never destroyed, in both modes. Refs #3844 * docs: add onConfigUpdate to the calls that do not hold the app unstable #3829 set onConfigUpdate to blockUntilFirst: false in tools/build.ts, because its observer runs only when a fetched template differs from the active one. The list in docs/zones.md left it out. Found in the #3835 review. Refs #3829 * docs(auth): say a second provideFirebaseApp can render signed out The bullet said a second registration makes a signed-in render fail on a freshly started server. With more than one registration, FirebaseApp resolves to the default app. Run on fresh servers, a second plain initializeApp(config) registration creates that app on the first request, so signed-in pages render signed out with no error. Only when no registration creates the default app, as with two initializeBrowserOrServerApp registrations, does the render fail, and only until a signed-out request creates it. Found in the #3835 review. Refs #3844
Fixes #3826
blockUntilFirst: falseentries totools/build.tsfor ten callback-taking calls whose first callback a normal load may never run.fromRefwith a hand-written one insrc/database/overrides.tsthat holds the app only for thevalueevent.isSupportedentry zone log verbosity #3613 meant forfirebase/remote-configinto the block that exports it.Checklist
isSupportedhold the app unstable until something that may never happen #382622.0.0-rc.1docs PR, which merges just before the release. Its first commit lists indocs/zones.mdthe calls that no longer hold the app unstable, and adds a sentence each understateChanges,auditTrailandfromRefindocs/database.md.yarn install,yarn testrun successfully?: yes, with npm:npm ci,npm run build,npm run build:jasmine,npm run test:node,npm run test:chrome-headless,npm run lint.Changes
The mechanism:
ɵzoneWrapregisters a pending task when a wrapped call is passed a callback.ApplicationRef.isStableis false: an SSR build's route extraction times out after 30 seconds (thebeforeAuthStateChangedimported from@angular/fire/authmakesng buildfail during route extraction #3748 failure), and anything waiting on stability in the browser waits with it.The ten calls whose first callback is not guaranteed, now
{ blockUntilFirst: false, logLevel: LogLevel.WARN }:onLog: never stable until some Firebase SDK logs. It returnsvoid, so no unsubscribe can release the task either.onTokenChanged: never stable until a token is obtained, which a first visit with no auto refresh and no protected request never asks for.onChildAdded: never stable on an empty node. A new listener's initial events are onlychild_added, one per existing child, andvalue.onChildChanged,onChildMoved,onChildRemoved: never stable until someone changes, moves or removes a child. None fires on attach.off(query, eventType, callback): never stable. It never calls the callback it is given and returnsvoid.enableLogging(fn): never stable until the database SDK logs something.stateChangesandauditTrail(rxfire, default events): never stable on an empty node, since they emit only on child events.logLevel: LogLevel.WARNmeans only the pending task changes: callbacks are still re-entered into the zone and the injection context, and a call outside an injection context still prints its per-function line at the warning level.fromRef(ref, event)in@angular/fire/database, now hand-written:blockUntilFirstvalue cannot fit both of its uses: thevalueevent always fires on attach, and a child event on an empty node never fires.src/database/overrides.tswraps rxfire'sfromReftwice at module load, once holding the app until the first emission and once not, and picks by event.tools/build.tsmarks it{ override: true }, so the generated file re-exports it.typeof fromRef.isSupportedin@angular/fire/remote-config, now{ blockUntilFirst: false }:true, every call in a browser with IndexedDB held a pending task until theindexedDB.openit awaits answered, and for good in a browser that never answers.isSupported: { blockUntilFirst: false }for it, matching the analytics and messaging entries, but in therxfire/remote-configblock, which exports noisSupported. Overrides match by name, so it did nothing.What callers can notice
onChildAddedchildren, so the server HTML of a list filled that way can ship before they arrive (reasoned from the wrapper, not run).onValue,object,listandfromRef(ref, ListenEvent.value)deliver a value on attach, empty node or not, and still hold the render.fromRefwith a child event no longer holds the render, matchingonChildAddedandstateChanges. To make a server render wait for a list's initial contents, uselist(ref)orlistVal(ref), which start with a one-time read and still hold it.off(ref, eventType, callback)no longer holds the app, but it still detaches nothing, because the wrapper replaces the callback with its own closure before the SDK compares callback identities.onValueandonChild*is the supported way to detach.ApplicationRef.whenStable()before chained Remote Config work no longer waits forisSupported.isSupported's.then,inject()throws and the zone is the one that attached the.then, at both flag values, so the callback sees no difference.isSupportedoutside an injection context now logs its per-call line only at the verbose level, as the analytics and messaging copies do.Verification
ApplicationRef.isStableemission and countedPendingTasks.addcalls.true, the never-firing cases above registered a task that was never released.true,onChildAddedon a populated node,onValue,onSnapshotsInSyncandonTokenChangedwith auto refresh on released theirs within 2 to 23 ms, andonChildChangedreleased as soon as a child was written.false, no task was registered in any run, and inside every callback that raninject()worked and, under zone.js,NgZone.isInAngularZone()was true.onLoginprovideAppInitializer:ng buildfails at the route extractor's 30-second timeout withRoutes extraction was abortedwhenonLogcomes from@angular/fire/app, and builds in about 2 s fromfirebase/app.isSupportedattrueholds the app unstable until the IndexedDB check answers, tens of milliseconds in Chrome, and for the whole window whenindexedDB.openis stubbed to never fire. Atfalseit registers no task.src/app/app.spec.ts,src/app-check/app-check.spec.tsandsrc/database/database.spec.ts, and 1 insrc/remote-config/remote-config.spec.ts. Each asserts whether the call registers a pending task.fromRefhas one spec per event kind.onLog,onChildChangedorisSupportedfromtools/build.tsand rebuilding fails the matching spec, and so does making the hand-writtenfromRefalways wait or never wait.npm run buildregenerates with the unrecognized-override check passing, andnpm run test:node,npm run test:chrome-headlessandnpm run lintpass.