Repository navigation
feat: accept firebase 13 alongside firebase 12 - #3831
armando-navarro wants to merge 2 commits into
Conversation
firebase 13.1.0 is npm latest, so `npm install firebase` followed by `ng add @angular/fire@22` failed with ERESOLVE against the ^12.18.0 peer. The peer range is now ^12.18.0 || ^13.0.0, and rxfire moves to ^6.3.0, whose peer range includes firebase 13. The ng add and ng update schematics now write the range of the firebase major already installed. ng add installs the missing peer at the newest allowed version before the schematic runs, so writing ^12.18.0 over an installed 13 left two @firebase/app copies and the app failed at startup. The lookup walks up to a monorepo root, and an installed copy the app's own entry excludes is ignored. The package keeps building against firebase 12, the oldest supported major, because the generated wrappers import functions by name from the installed firebase. A new CI job runs the specs against firebase 13.1.0 on Node 24, which firebase 13 requires. Fixes angular#3830
…ecursive option Node 26 removed the recursive option of fs.rmdirSync, so the spec failed in the Node 26 CI jobs. Delete the files and directories one by one instead, as duplicatePackages.jasmine.ts already does.
tyler-reitz
left a comment
There was a problem hiding this comment.
One blocking point: the monorepo spec's cleanup throws on Node 26, which is what fails all three Test Node 26 jobs.
rmdirSync(repository, { recursive: true }) is rejected there with The property 'options.recursive' is no longer supported; Node 24 only warns (DEP0147). Locally on Node 26, test:node reports 401 specs, 1 failure, "finds firebase installed at a monorepo root above the workspace", and the temp directory is left behind. The same tree on Node 24 passes 401/401. With @types/node at 12.12.41 there's no typed rmSync, so either a types bump or a cast; your call.
On the installed-version lookup you asked about: it reads _backend._root the same way setup/index.ts does, and walking up matches how Node resolves the package. Neutering the walk-up, the installed-version read, and the excluded-copy guard each turns its spec red, so the coverage holds. The one thing I'd note is that the walk-up has no stopping point: with no in-tree firebase and no firebase entry, a stray copy in an ancestor such as ~/node_modules decides the range. Node would resolve that same copy at runtime, so I read it as low risk (inferred, not run).
Everything else checked out, including the firebase 13 job and both browser suites.
|
Thanks, Tyler. The Node 26 failure is fixed in 5c5a8b9. I went with neither a types bump nor a cast: the spec now deletes its temporary directory file by file, the way That also removes the DEP0147 warning the old cleanup printed on Node 22 and 24. Locally, On the walk-up, I agree it's low risk, so I've left it as is. It only reaches above the app when the app's own Could you take another look? |
tyler-reitz
left a comment
There was a problem hiding this comment.
Approving. The removeDirectory helper fixes the Node 26 failure: all three Test Node 26 jobs pass on 5c5a8b9, along with the firebase 13 job and both browser suites.
Fixes #3830
docs/version-22-upgrade.mdchanges go on the single rc.1 docs pull request, which merges just before the tag.src/schematics/common.jasmine.ts.yarn install,yarn testrun successfully? yes, with npm:npm ci,npm run build,npm run lint,npm run test:nodeandnpm run test:chrome-headless.Description
This lets
@angular/fire22 accept firebase 13 alongside 12, song addstops failing withERESOLVEon apps that already have firebase 13. The reasoning is in the linked issue.Changes
src/package.json:firebaseis^12.18.0 || ^13.0.0.rxfireis^6.3.0.alignFirebaseVersioninsrc/schematics/common.ts, used byng addand the v21 and v22 migrations:^12.18.0 || ^13.0.0alone.node_modules/firebasein the workspace and then in each parent directory, so it also finds a monorepo root install.^13.0.0, and anything else gets^12.18.0..github/workflows/test.ymlhas a new "Test firebase 13" job:test:nodeandtest:chrome-headlesson Node 24, with firebase 13.1.0 installed over the firebase 12 build.test_and_contributeandpublishboth wait on it.Behavior to know
ng add @angular/fireon a new app with no firebase now writes^13.0.0, because npm has already installed firebase 13 as the missing peer.^12.18.0, so the app stays on 12.node_modules/firebase, so a new app on pnpm gets^12.18.0and one copy of firebase 12.idFieldonto the object a Firestore converter returns, instead of onto a copy.idFieldset, a converter that returns a frozen object now throws incollectionDataanddocData.^6.2.0already resolves to 6.3.0.Verified outside CI
ng addin fresh Angular 22 apps ended with one copy of firebase in every starting state I tried: no firebase, firebase 12 or 13 already installed, and entries like>=13.0.0,*and a firebase 13 prerelease.ng updatefrom Angular 21 to 22 kept each app on its existing firebase major.Tyler, the part I'd most like a second look at is the installed-version lookup in
alignFirebaseVersion, since it reads outside the schematic tree.