feat(schematics): warn during ng add when firebase is installed at more than one version - #3760
Conversation
…rejected dependency workspace.ts holds what more than one schematic needs to know about a user's workspace on disk: the lockfile-to-manager table, tolerant JSON readers, the upward walk that finds the directory owning an install, and assertSafeDependencyName, moved from deploy/actions.ts unchanged. Files are parsed with jsonc-parser, as utils.ts already does: Angular tolerates comments in angular.json, and strict parsing silently dropped a commented file's cli.packageManager declaration. The upward walk also stops at bun.lock, bun.lockb and deno.lock, which mark the directory owning an install even though nothing here can query those managers. assertSafeDependencyName gains a required source parameter naming where the value came from. A rejected name is useless to a user who is not told which file to go and edit, and the deploy spec pins that its error still points at angular.json.
…its own package manager
A package installed at two versions is two module instances, and they reject
each other's objects at runtime with errors naming the caller's code. The
reports that reach this repo are diagnosed by telling the reporter to run
npm ls firebase. This module runs that question itself.
One file per manager: npm, pnpm, yarn 2+ and yarn 1.x each get their command
and their reader, since the four output formats share nothing. index.ts
identifies the workspace's manager from its own declarations before its
lockfiles, and tells the two yarns apart by the lockfile's own header
('# yarn lockfile v1' against an __metadata: block), probing yarn --version
only when no lockfile is readable: the binary on PATH and the project
disagree in corepack's default state. The query runs through one spawn
wrapper (cross-spawn, argument array, no shell) and reports entries,
distinct versions and problems without rendering any verdict.
Finding nothing is ambiguous, so every path that cannot reach an answer
records a problem: silence downstream has to mean checked and fine.
Parsing specs run against output captured verbatim from real installs of all
four managers. One spec launches npm for real, which is the part captured
output cannot cover and the part that fails first on Windows.
Known limit, deliberate: in a monorepo the question is answered for the whole
workspace while the caller was pointed at one project inside it, so a project
resolving one version can be warned about a sibling's.
…re than one version Fixes angular#3754. ng add runs the check after the install task and before the feature prompt, so node_modules is on disk to be asked about and the warning appears whatever the user selects. duplicateWarning.ts owns the verdict the reporter refuses to render. It says one of three things: more than one version found, with each version's dependency chain named so the user can see what pulled the second copy in; the check could not be completed, with the reasons; or nothing, which a reader may take as checked and fine. The whole body is inside one try, because this is a courtesy and an exception here must not abort ng add. Verified end to end against the built tarball: a fresh Angular 21 app with a planted second firebase warns between the install and the prompt, and the same app without the duplicate prints nothing.
tyler-reitz
left a comment
There was a problem hiding this comment.
Approving. I checked the parts only running can settle.
The real-spawn spec really does spawn: pointing executableFor at a binary that doesn't exist turns exactly that spec red, so it isn't quietly short-circuiting into the captured-output path. 314 specs, 0 failures here.
The no-silent-failure rule holds across all four managers. Feeding each parser nonsense gives npm and pnpm a throw, which findInstalledCopies converts into a problem, and both yarns a 2 lines of the output could not be read problem. None of them returns an empty answer that would read as clean.
assertSafeDependencyName moving into workspace.ts is behavior-preserving, which felt worth checking properly since it's the guard from the injection fixes: same function object re-exported, 17 hostile names still rejected, 4 legitimate ones still accepted, and the message at the deploy call site is byte-identical.
cross-spawn isn't in src/package.json, which looked like #3747 again until I checked the output. esbuild inlines it, no require("cross-spawn") in the built bundle, so it's fine as is.
One note, not blocking: defaultTimeoutMs is 30 seconds and nothing is printed while the query runs, so a wedged package manager buys the user half a minute of silence between the install and the prompt.
Fixes #3754
Refs #3684
Refs #3681
Refs #3682
What this does
ng add @angular/firenow asks the project's own package manager which versions offirebaseare installed, and says one of three things: more than one version was found, the check could not be completed, or nothing at all.It runs after the install task and before the feature prompt, so
node_modulesis on disk to be asked about and the answer appears whatever the user then selects.Why
Two copies of the Firebase SDK produce two separate module instances, and objects made by one are rejected by the other. The symptoms name the user's own code, so the cause is not guessable from the error:
Expected type '_Query', but it was: a custom _Collection objectType does not match the expected instance#3684, #3681 and #3682 are all this, and all open. In each case the diagnosis came down to one command a maintainer had to tell the reporter to run,
npm ls firebase. This runs it for them.What a user sees
Real
ng addoutput, from a fresh Angular 21 app withfirebase@12.18.0at the root andfirebase@11.10.0under a local dependency:The
vialines are the part worth having: they name the dependency that pulled the second copy in, which is the fact a user needs in order to act.On the same app without the duplicate,
ng addprints nothing between the install and the prompt.The rule about silence
Printing nothing is itself a message: a user who reads nothing concludes the install was inspected and is fine. So every path that cannot reach an answer says so instead of staying quiet. A missing package manager, a lockfile written by a newer version, a manager that exits without printing, a project declaring bun, output in an unrecognized shape: each produces a second kind of warning that names what stopped the check.
How it works
packageManagerinpackage.json,cli.packageManagerinangular.json), then from its lockfile. Both yarns shareyarn.lock, so the lockfile's own first lines decide which (# yarn lockfile v1for yarn 1.x, a__metadata:block for yarn 2+), with ayarn --versionprobe as the fallback when no lockfile is readable.npm ls,pnpm -r ls,yarn why,yarn list.setup/duplicateWarning.ts, which is the only place that decides what to say.Commands run through
cross-spawnwith an argument array and noshell: true.Known limit
In a monorepo the question is answered for the whole workspace while
ng addwas pointed at one project inside it. A project that resolves exactly one version offirebasecan therefore be warned about a sibling's version that it does not depend on and cannot load.This is not fixed here. Scoping the answer is per-manager work: npm scopes if the command runs in the project's own directory, pnpm scopes if
-ris dropped as well, and neither yarn scopes by directory at all. The message is worded so it is not false in that case (it explains what two copies loaded by one app do, rather than asserting a conflict has happened) and thevialines name which project each version came through.Tests
overridesandresolutions, locally linked packages, yarn 2+ virtual locators, and a yarn 1.x project with yarn 2+ on the PATH.Not included
ng add. This does not run at build time or as a lint rule.