build: require firebase ^12.18.0 and wrap the functions it adds - #3761
Conversation
tyler-reitz
left a comment
There was a problem hiding this comment.
Approving. On your register question: leave it. The reason is stronger than "getToken has done the same for years": register and getToken have identical permission preambles in @firebase/messaging 12.18.0, both await Notification.requestPermission() when permission is default, then throw permission-blocked. It isn't a new hazard, it's the same one, now on the API that replaces the one already carrying it. Your reasoning about the alternative holds too: at blockUntilFirst: false a promise falls to zones.ts:153-161 and comes back bare, losing the runInInjectionContext re-entry that 169-180 gives it. There's no "re-enter but don't block" setting.
Ran the rest. npm ci at 12.18.0 then npm run generate reproduces the committed files byte for byte, so messaging really is the only one that moves. 227 specs, 0 failures. Forcing alreadyCompatible = false turns both rewritten specs red, so they're live; simulating the next raise to ^12.20.0 leaves them passing and trips only the pre-existing src/package.json drift guard, which is the right thing to fire.
One follow-up, not for this PR: 12.18.0 deprecates both getToken and deleteToken in favour of register/onRegistered/onUnregistered, and docs/messaging.md:81 still imports both. Worth an issue.
Also, this branch carries #3759's three commits, so it'll want a rebase once that squash-merges.
The generated src/<module>/firebase.ts files are written from the type declarations of whatever firebase is installed, so the set of functions AngularFire wraps is decided by the version the build resolves. That was 12.4.0 while npm latest reached 12.18.0, leaving newly added functions reaching callers through the star export with no zone integration and no pending-task registration. Raising the required version wraps them. Everything the override entries in angular#3759 classified as unwrapped stays out. getImagenModel loses its entry because firebase removed the symbol in 12.18.0, so the generator no longer sees it. The exemption list it sat in is empty now and stays as the place the next unclassified name goes. Fixes angular#3756
89ab184 to
128bfda
Compare
firebase 12.18 deprecates getToken and deleteToken and says both will be removed, pointing callers at register with onRegistered, and unregister with onUnregistered. The replacement is a different model rather than a rename, so the page now explains it before the example. The Node send example keeps its token field, which Firebase says still accepts an installation ID during the migration. It does not switch to the fid field Firebase recommends, because firebase-admin 13.5.0 does not declare one.
The comment claimed a name allowed for one entry point is not allowed for the rest. The check does not work that way. exportsSeenPerOverrides is keyed by the overrides object, and firestore and firestore/lite are handed the same firestoreOverrides, so a name listed for one silences the check for the other. The list is empty today, so nothing behaves differently. The comment is what a future reader would have relied on when adding the next name.
|
On the docs point, I have updated the docs here rather than filing it, since the PR template asks whether docs are included and this is the change that makes the replacement calls available. Pushed as a follow-up commit. Two things I found while writing it that are worth knowing:
|
Checklist
firebaseversion AngularFire is built against, so functions added since 12.4.0 are wrapped #3756 (required)docs/version-21-upgrade.md,docs/messaging.md)yarn install,yarn testrun successfully? yes, via the npm equivalentsDescription
tools/build.tswritessrc/<module>/firebase.tsfrom the type declarations of whateverfirebaseis installed, so which functions@angular/firewraps is decided by the version the build resolves rather than by a list anyone maintains. This raises that version, and regenerates. #3756 has the background.Three things follow from the way the generator works:
latestreached 12.18.0.export *line, but arrives with none of the wrapper's behavior and nothing saying it is different.alignFirebaseVersionstarts writing a range the library does not install against.What regenerating changes
src/messaging/firebase.tsis the only generated file that moves. It gainsonRegistered,onUnregistered,registerandunregister.export *line continues to carry them to callers.What comes out
getImagenModelcarried an override entry so the generator would not emit a named import for a symbol firebase deleted in 12.18.0 (@angular/fire/aifails to build against firebase 12.18, which removedgetImagenModel#3749, fixed by fix(ai): drop the getImagenModel wrapper, removed in firebase 12.18 #3750).overridesFirebaseDoesNotDeclare, which stays in place as the list the next unclassified name goes into.What this means for someone upgrading
@angular/firedeclaresfirebaseas a regular dependency rather than a peer, so a workspace holding an olderfirebasethat upgrades@angular/firewithout running a schematic now resolves two copies instead of one, and the two reject each other's objects at runtime. Measured under Testing rather than assumed.alignFirebaseVersionrewrites the declared range for anyone who runsng addorng update, and it picks up the new range with no further change.ng addshould warn when the workspace has more than one version offirebaseinstalled #3754 covers telling the people who run neither.Specs
alignFirebaseVersionspecs used^12.6.0to stand for a range a workspace declares that is already inside what the library requires.^12.6.0stops being inside that range at 12.18.0, so both specs would have started asserting the opposite of what they say.firebaseVersionRange, so the next raise cannot leave them wrong.Docs
docs/messaging.mdwrote the firebase version into two gstatic service worker URLs, which is only correct on the day it is written. It now uses a placeholder, matchingdocs/compat/messaging/messaging.md.docs/version-21-upgrade.mdnames the new range, and no longer implies that only afirebase11 workspace ends up with two copies.Testing
npx ng lintpasses.Two results are worth stating separately, because neither is visible in the diff.
The override entries are doing the work, not the version
functionin the installed firebase 12.18.0, so their absence is those entries working rather than the version simply not having them.getImagenModelisundefinedthere, which is why its entry could go.The duplicate copy was measured, not assumed
A fresh
ng newapp,firebaseset to 12.10.0, this build of@angular/fireinstalled, no schematic run:The split runs through the library's own dependencies:
@angular/firegets 12.18.0 whilerxfire, which@angular/firedepends on, gets the app's 12.10.0. The same app with nofirebaseof its own resolves a single 12.18.0 for everything.One judgment call I would particularly like a second opinion on:
registeris wrapped withblockUntilFirstat its default, which means a call made before notification permission is granted holds the application unstable while the browser prompt is open.getTokenhas done the same for years, and settingfalsewould drop zone and injection-context re-entry from the returned promise rather than just the pending task, so I left it alone. If you would rather that not ship, say so and I will take it out of this PR.Fixes #3756