Skip to content

Commit e578ccb

Browse files
build: classify the firebase symbols a version raise would newly wrap (#3759)
The build regenerates src/<module>/firebase.ts from the declarations of whatever firebase is installed, wrapping every lowercase-initial function. Raising the minimum version would wrap symbols nobody has classified, and one breaks: the auth persistence exports are classes that firebase constructs with `new`, and the wrapper replaces them with an ordinary function. These entries settle that first and stay inert until the version rises. Do not wrap maximum, minimum, getTemplateGenerativeModel, makeMemoryCacheProvider or browserCookiePersistence. Wrap the messaging subscriptions with blockUntilFirst off, matching onMessage, since they fire on a registration change that may never happen and would otherwise hold a pending task open indefinitely. The generator is now exported so `npm run generate` runs it without `ng build`, and it fails on an override key its entry point does not declare, which is otherwise ignored in silence. Also removes ngPostUpdate, an ng update migration that returned its Tree unchanged, and adds a docs/zones.md section on what a call outside an injection context loses. Refs #3756
1 parent b83343d commit e578ccb

6 files changed

Lines changed: 79 additions & 26 deletions

File tree

docs/zones.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,19 @@ readonly refreshed$ = this.refresh$.pipe(switchMap(() => this.items$));
4949

5050
Reach for `runInInjectionContext` only when the call genuinely must run inside the callback, as the per-user query above does (it needs the signed-in user's `uid`).
5151

52+
### Why the injection context is required
53+
54+
AngularFire cannot create an injection context, only borrow the one you are already in.
55+
56+
When you call a wrapped API, the first thing AngularFire does is ask Angular for three things with `inject()`: its own scheduler service, Angular's `PendingTasks` register, and an `EnvironmentInjector`. `inject()` only works inside an injection context, so outside of one the first of them throws and the rest are never reached. AngularFire catches that, warns while in dev-mode as described under Logging below, and calls the Firebase API directly with nothing added.
57+
58+
That division is worth knowing, because it explains what you are and are not responsible for:
59+
60+
* **You** supply the context at the call site, from a field initializer, a constructor, or an explicit `runInInjectionContext`.
61+
* **AngularFire** re-enters the environment injector inside a callback you hand to the call itself, which is why you never wrap an `onSnapshot` handler yourself. That reaches anything provided at the application level, but not providers declared on a component, and it does not extend to subscribers of a returned Observable, which is what the `switchMap` and `runInInjectionContext` example above is for.
62+
63+
What you lose depends on the call. A call that does asynchronous work, such as `onSnapshot`, `getDoc` or `collectionData`, is normally added to Angular's `PendingTasks` register, which is what server-side rendering waits on before it serializes the page. Called outside a context, it never is. A call that returns immediately, such as `getFirestore`, was never registered anyway, so it loses only the zone handling.
64+
5265
## Logging
5366

5467
You may see a log warning, `Calling Firebase APIs outside of an Injection context may destabilize your application leading to subtle change-detection and hydration bugs. Find more at https://github.com/angular/angularfire/blob/main/docs/zones.md` when developing your application.

package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
"test:build": "bash ./test/ng-build/build.sh",
2424
"test:all": "npm run test:node-esm && npm run test:chrome-headless && npm run test:typings && npm run test:build",
2525
"build": "rimraf dist && tspc -p tsconfig.build.json && node --trace-warnings ./tools/build.js && npm pack ./dist/packages-dist",
26+
"generate": "tspc -p tsconfig.build.json && node -e \"require('./tools/build.js').zoneWrapExports().catch(err => { console.error(err); process.exit(1); })\"",
2627
"buildd": "tsc -p tsconfig.build.json && node --trace-warnings ./tools/build.js && npm pack ./dist/packages-dist",
2728
"build:jasmine": "npx tsc -p tsconfig.jasmine.json --module es2015 && cp ./dist/out-tsc/jasmine/tools/jasmine.js ./dist/out-tsc/jasmine/tools/jasmine.mjs && npx tsc -p tsconfig.jasmine.json && cp ./dist/packages-dist/schematics/versions.json ./dist/out-tsc/jasmine/src/schematics",
2829
"changelog": "conventional-changelog -p angular -i CHANGELOG.md -s -r 1"

src/schematics/migration.json

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,6 @@
1010
"version": "21.0.0",
1111
"description": "Align the workspace's firebase dependency with the range @angular/fire 21 requires, and rewrite Vertex AI imports to Firebase AI Logic (getVertexAI callers keep the Vertex AI backend)",
1212
"factory": "./update/v21#ngUpdate"
13-
},
14-
"ng-post-upgate": {
15-
"description": "Print out results after ng-update",
16-
"factory": "./update#ngPostUpdate",
17-
"private": true
1813
}
1914
}
2015
}

src/schematics/tsconfig.json

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
"outDir": "../../dist/packages-dist/schematics"
2323
},
2424
"files": [
25-
"update/index.ts",
2625
"deploy/actions.ts",
2726
"deploy/builder.ts",
2827
"add/index.ts",

src/schematics/update/index.ts

Lines changed: 0 additions & 8 deletions
This file was deleted.

tools/build.ts

Lines changed: 65 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,13 @@ const firestoreOverrides = {
3737
increment: { logLevel: LogLevel.VERBOSE },
3838
limit: { logLevel: LogLevel.VERBOSE },
3939
limitToLast: { logLevel: LogLevel.VERBOSE },
40+
// maximum and minimum take no callback and return no promise; wrapping would not benefit them.
41+
maximum: null,
4042
memoryEagerGarbageCollector: null,
4143
memoryLocalCache: null,
4244
memoryLruGarbageCollector: null,
45+
// See maximum above.
46+
minimum: null,
4347
namedQuery: { logLevel: LogLevel.VERBOSE },
4448
or: { logLevel: LogLevel.VERBOSE },
4549
orderBy: { logLevel: LogLevel.VERBOSE },
@@ -61,14 +65,38 @@ const firestoreOverrides = {
6165
writeBatch: { logLevel: LogLevel.VERBOSE },
6266
};
6367

68+
/* Override keys firebase's type declarations may not carry, which the generator would otherwise
69+
* read as typos. A name lands here because a later release adds it, firebase has removed it, or
70+
* firebase keeps it out of its public types on purpose (browserCookiePersistence). Listing a
71+
* declared name is harmless: getImagenModel is here for the version this build is heading to.
72+
* Keyed by entry point, so a name allowed for one module is not allowed for the rest.
73+
* At a version bump, drop a name from here but KEEP its override entry. */
74+
const overridesFirebaseDoesNotDeclare: Record<string, string[]> = {
75+
'firebase/ai': ['getImagenModel', 'getTemplateGenerativeModel'],
76+
'firebase/auth': ['browserCookiePersistence'],
77+
'firebase/data-connect': ['makeMemoryCacheProvider'],
78+
'firebase/firestore': ['maximum', 'minimum'],
79+
'firebase/messaging': ['onRegistered', 'onUnregistered', 'unregister'],
80+
};
81+
82+
type Overrides = Record<string, OverrideOptions | null>;
83+
6484
function zoneWrapExports() {
85+
/* One overrides object can be applied to several entry points, so the keys it may legally name
86+
* are the union of their export lists. firestoreOverrides covers both firestore and
87+
* firestore/lite, and only firestore has persistentLocalCache and the rest. */
88+
const exportsSeenPerOverrides = new Map<Overrides, Set<string>>();
6589
const reexport = async (
6690
module: string,
6791
name: string,
6892
path: string,
6993
exports: string[],
70-
overrides: Record<string, OverrideOptions | null> = {}
94+
overrides: Overrides = {}
7195
) => {
96+
const seen = exportsSeenPerOverrides.get(overrides) ?? new Set<string>();
97+
exports.forEach(exportName => seen.add(exportName));
98+
(overridesFirebaseDoesNotDeclare[path] ?? []).forEach(undeclared => seen.add(undeclared));
99+
exportsSeenPerOverrides.set(overrides, seen);
72100
const imported = await import(path);
73101
const toBeExported: [string, string, boolean][] = exports.sort().
74102
filter(it => !it.startsWith('_') && overrides[it] !== null && overrides[it]?.override !== true).
@@ -113,12 +141,23 @@ ${exportedZoneWrappedFns}
113141
`;
114142
await writeFile(filePath, fileOutput);
115143
};
144+
const failOnUnrecognizedOverrides = () => {
145+
const unrecognized = [...exportsSeenPerOverrides].flatMap(([overrides, seen]) =>
146+
Object.keys(overrides).filter(key => !seen.has(key) && overrides[key]?.override !== true));
147+
if (unrecognized.length) {
148+
throw new Error(
149+
`Override keys their entry point does not declare: ${unrecognized.join(', ')}. ` +
150+
'Overrides are matched by name, so this one is silently ignored and the symbol keeps the ' +
151+
'default. Fix the spelling, move it to the right block, or add it to ' +
152+
'overridesFirebaseDoesNotDeclare.'
153+
);
154+
}
155+
};
116156
return Promise.all([
117157
reexport('ai', 'firebase', 'firebase/ai', tsKeys<typeof import('firebase/ai')>(), {
118-
// Removed in @firebase/ai 2.15.0 (firebase 12.18.0), which the ^12.4.0 range
119-
// resolves for fresh installs. A named import here would make consumer builds
120-
// fail on that version, so only re-export it through the star export, which
121-
// tracks whichever firebase is installed.
158+
// Unwrapped via the star export: no callback, returns the model object directly.
159+
getTemplateGenerativeModel: null,
160+
// Removed in @firebase/ai 2.15.0 (firebase 12.18.0).
122161
getImagenModel: null,
123162
}),
124163
reexport('analytics', 'firebase', 'firebase/analytics', tsKeys<typeof import('firebase/analytics')>(), {
@@ -138,7 +177,11 @@ ${exportedZoneWrappedFns}
138177
reexport('auth', 'rxfire', 'rxfire/auth', tsKeys<typeof import('rxfire/auth')>()),
139178
reexport('auth', 'firebase', 'firebase/auth', tsKeys<typeof import('firebase/auth')>(), {
140179
debugErrorMap: null,
180+
/* These 5 persistence entries MUST stay unwrapped. Though their type declarations
181+
* disagree, they are classes, and wrapping would replace them with ordinary functions
182+
* that throw when used with `new`. */
141183
inMemoryPersistence: null,
184+
browserCookiePersistence: null,
142185
browserLocalPersistence: null,
143186
browserSessionPersistence: null,
144187
indexedDBLocalPersistence: null,
@@ -209,6 +252,8 @@ ${exportedZoneWrappedFns}
209252
update: { logLevel: LogLevel.VERBOSE },
210253
}),
211254
reexport('data-connect', 'firebase', 'firebase/data-connect', tsKeys<typeof import('firebase/data-connect')>(), {
255+
// Unwrapped via the star export: no callback, returns a plain settings value.
256+
makeMemoryCacheProvider: null,
212257
mutationRef: { logLevel: LogLevel.VERBOSE },
213258
queryRef: { logLevel: LogLevel.VERBOSE },
214259
toQueryRef: { logLevel: LogLevel.VERBOSE },
@@ -225,10 +270,14 @@ ${exportedZoneWrappedFns}
225270
reexport('messaging', 'firebase', 'firebase/messaging', tsKeys<typeof import('firebase/messaging')>(), {
226271
isSupported: { blockUntilFirst: false },
227272
onMessage: { blockUntilFirst: false },
273+
// `blockUntilFirst: false` otherwise `ApplicationRef.isStable` may never become `true`.
274+
onRegistered: { blockUntilFirst: false },
275+
onUnregistered: { blockUntilFirst: false },
228276
deleteToken: { logLevel: LogLevel.VERBOSE },
277+
// Quiets the per-call log line, matching deleteToken above.
278+
unregister: { logLevel: LogLevel.VERBOSE },
229279
}),
230280
reexport('remote-config', 'rxfire', 'rxfire/remote-config', tsKeys<typeof import('rxfire/remote-config')>(), {
231-
isSupported: { blockUntilFirst: false },
232281
getValue: { exportName: 'getValueChanges' },
233282
getString: { exportName: 'getStringChanges' },
234283
getNumber: { exportName: 'getNumberChanges' },
@@ -259,7 +308,7 @@ ${exportedZoneWrappedFns}
259308
collection: { exportName: 'collectionSnapshots' },
260309
}),
261310
reexport('firestore/lite', 'firebase', 'firebase/firestore/lite', tsKeys<typeof import('firebase/firestore/lite')>(), firestoreOverrides),
262-
]);
311+
]).then(failOnUnrecognizedOverrides);
263312
}
264313

265314
const src = (...args: string[]) => join(process.cwd(), 'src', ...args);
@@ -331,7 +380,6 @@ function spawnPromise(command: string, args: string[]) {
331380
// Path segments of each schematic entry point, relative to `schematics/` and without the file
332381
// extension: esbuild compiles the `.ts` and loadCompiledSchematics requires the emitted `.js`.
333382
const schematicEntryPoints = [
334-
['update', 'index'],
335383
['deploy', 'actions'],
336384
['deploy', 'builder'],
337385
['add', 'index'],
@@ -409,7 +457,12 @@ async function buildLibrary() {
409457
]);
410458
}
411459

412-
buildLibrary().catch(err => {
413-
console.error(err);
414-
process.exit(1);
415-
})
460+
// Exported so `npm run generate` can run the code generation on its own.
461+
export { zoneWrapExports };
462+
463+
if (require.main === module) {
464+
buildLibrary().catch(err => {
465+
console.error(err);
466+
process.exit(1);
467+
});
468+
}

0 commit comments

Comments
 (0)