Repository navigation
fix: stop twelve wrapped calls from holding the app unstable - #3827
armando-navarro wants to merge 1 commit into
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.
Fixes #3826
blockUntilFirst: falseentries totools/build.tsfor eleven callback-taking calls whose first callback a normal load may never run.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 #3826docs/zones.mdthat says callback APIs do.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 eleven 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.fromRefon a child event,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.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,objectandlistdeliver a value on attach, empty node or not, and still hold the render.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 the call registers no pending task.onLog,onChildChanged,fromReforisSupportedfromtools/build.tsand rebuilding fails the matching spec.npm run buildregenerates with the unrecognized-override check passing, andnpm run test:node,npm run test:chrome-headlessandnpm run lintpass.