Skip to content

Commit 3a5b2bb

Browse files
fix(schematics): move firestore starter files after init, widen error handling
Tyler's review on #3714: createFirestoreStarterFiles staged the Tree before the DataConnect init calls, using a pre-init firebase.json snapshot. If a future init call ever added a firestore section mid-run, the stale snapshot would miss it and stage starter files on top of files firebase-tools already wrote to disk — the same Tree/disk collision the .firebaserc write hit earlier in this PR. Moved the call to run after init with a fresh re-read, and documented the more immediate invariant this enforces: it must run before addFirestoreToFirebaseJson, which is what adds the firestore section on a normal run. Also widened addFirestoreToFirebaseJson's try/catch to cover the firebase.json mutation and write, not just the read — a write failure (disk full, permissions) previously crashed with a raw Node error instead of the warn-and-continue the read path already gets.
1 parent 702a0cb commit 3a5b2bb

3 files changed

Lines changed: 41 additions & 12 deletions

File tree

src/schematics/setup/firebaseConfigs.jasmine.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,4 +165,12 @@ describe('addFirestoreToFirebaseJson', () => {
165165
expect(warnSpy).toHaveBeenCalled();
166166
});
167167

168+
it('warns and skips when firebase.json parses to a non-object', () => {
169+
writeFileSync(join(projectRoot, 'firebase.json'), 'null');
170+
const warnSpy = spyOn(context.logger, 'warn');
171+
addFirestoreToFirebaseJson(projectRoot, context, [FEATURES.Firestore]);
172+
expect(warnSpy).toHaveBeenCalled();
173+
expect(readFileSync(join(projectRoot, 'firebase.json')).toString()).toBe('null');
174+
});
175+
168176
});

src/schematics/setup/firebaseConfigs.ts

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,12 @@ export const setDefaultProjectInFirebaseRc = (projectRoot: string, projectId: st
6262
* whose firebase.json already has a `firestore` section is left entirely untouched: its section
6363
* may point at differently-named files, and creating the default-named ones would only add
6464
* orphans nothing references.
65+
*
66+
* `firebaseJsonConfig`, when passed, should be the post-init snapshot of firebase.json (read
67+
* after the last `firebaseTools.init` call and before `addFirestoreToFirebaseJson` runs) — see
68+
* the call site in `ngAddSetupProject` for why that order matters. Passing a stale pre-init
69+
* snapshot risks staging the default-named starter files on top of files firebase-tools already
70+
* wrote to disk, colliding when the Tree commits.
6571
*/
6672
export const createFirestoreStarterFiles = (
6773
host: Tree,
@@ -101,19 +107,22 @@ export const addFirestoreToFirebaseJson = (
101107
features: FEATURES[],
102108
) => {
103109
if (!features.includes(FEATURES.Firestore)) { return; }
104-
// Re-read first: firebaseTools.init calls may have rewritten firebase.json on disk.
105-
let firebaseJson: FirebaseJSON;
110+
// firebaseTools.init calls may have rewritten firebase.json on disk, so re-read it, add the
111+
// firestore section if absent, and write it back. The whole read-check-write is guarded: a
112+
// failed read (missing or unparseable file), a parse that isn't a usable object (reading
113+
// `.firestore` off `null`/`undefined` throws; assigning it on a non-object primitive throws
114+
// too), or a failed write (disk full, permissions) all warn and leave the file for the user to
115+
// finish, rather than crashing the schematic with a raw Node error.
106116
try {
107-
firebaseJson = JSON.parse(readFileSync(join(projectRoot, 'firebase.json')).toString());
117+
const firebaseJson: FirebaseJSON = JSON.parse(readFileSync(join(projectRoot, 'firebase.json')).toString());
118+
if (!firebaseJson.firestore) {
119+
firebaseJson.firestore = { rules: 'firestore.rules', indexes: 'firestore.indexes.json' };
120+
writeFileSync(join(projectRoot, 'firebase.json'), stringifyFormatted(firebaseJson));
121+
}
108122
} catch (e) {
109123
context.logger.warn(
110-
`Could not read firebase.json to add the firestore section (${e.message}). Add ` +
111-
'{ "firestore": { "rules": "firestore.rules", "indexes": "firestore.indexes.json" } } to it manually.'
124+
`Could not update firebase.json with the firestore section (${e.message}). Check firebase.json ` +
125+
'and add { "firestore": { "rules": "firestore.rules", "indexes": "firestore.indexes.json" } } manually.'
112126
);
113-
return;
114-
}
115-
if (!firebaseJson.firestore) {
116-
firebaseJson.firestore = { rules: 'firestore.rules', indexes: 'firestore.indexes.json' };
117-
writeFileSync(join(projectRoot, 'firebase.json'), stringifyFormatted(firebaseJson));
118127
}
119128
};

src/schematics/setup/index.ts

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,8 +90,6 @@ export const ngAddSetupProject = (
9090

9191
const firebaseProject = await projectPrompt(defaultProjectName, { projectRoot, account: user.email });
9292

93-
createFirestoreStarterFiles(host, context, features, firebaseJson);
94-
9593
let firebaseApp: FirebaseApp|undefined;
9694
let sdkConfig: Record<string, string>|undefined;
9795

@@ -149,6 +147,20 @@ export const ngAddSetupProject = (
149147

150148
}
151149

150+
// Re-read firebase.json after the last firebaseTools.init call — init rewrites it on disk
151+
// mid-run, staging files against a stale pre-init copy risks the same Tree/disk collision
152+
// .firebaserc hit earlier in this file (a Tree-staged file colliding with one firebase-tools
153+
// already wrote to disk, which crashes the schematic when the Tree commits). Right now no
154+
// init call adds a firestore section, so this is defensive against a future one that might.
155+
const firebaseJsonAfterInit: FirebaseJSON = JSON.parse(
156+
readFileSync(join(projectRoot, "firebase.json")).toString()
157+
);
158+
// Must run before addFirestoreToFirebaseJson below: that call is what adds the firestore
159+
// section on a normal run. If it ran first, this read would see the section it just added
160+
// and skip creating the files — leaving firebase.json pointing at rules/indexes files that
161+
// were never created.
162+
createFirestoreStarterFiles(host, context, features, firebaseJsonAfterInit);
163+
152164
// Both write the real filesystem, after the last firebaseTools.init call — init writes
153165
// .firebaserc and rewrites firebase.json on disk during the run.
154166
setDefaultProjectInFirebaseRc(projectRoot, firebaseProject.projectId);

0 commit comments

Comments
 (0)