Repository navigation
fix(emulator): keep firebase-admin statics when proxying; don't crash logging circular thrown values - #11460
fix(emulator): keep firebase-admin statics when proxying; don't crash logging circular thrown values#11460ayushflow wants to merge 1 commit into
Conversation
…rash logging circular errors Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request addresses two issues: it preserves static properties of firebase-admin namespaces when functions are bound in the Functions emulator, and it prevents crashes when logging circular references by falling back to util.inspect. The review feedback recommends avoiding the use of 'any' in tests by using Object.assign, skipping 'arguments' and 'caller' properties when copying descriptors to prevent strict mode crashes, and ensuring getErrMsg always returns a string even if JSON.stringify returns undefined.
| const firestore: any = () => "instance"; | ||
| firestore.Timestamp = class Timestamp {}; | ||
| firestore.FieldValue = { serverTimestamp: () => "ts" }; |
There was a problem hiding this comment.
To adhere to the repository style guide which states 'Never use any or unknown as an escape hatch', we can avoid using any here by using Object.assign to combine the function and its static properties. This allows TypeScript to infer the correct intersection type automatically.
| const firestore: any = () => "instance"; | |
| firestore.Timestamp = class Timestamp {}; | |
| firestore.FieldValue = { serverTimestamp: () => "ts" }; | |
| const firestore = Object.assign(() => "instance", { | |
| Timestamp: class Timestamp {}, | |
| FieldValue: { serverTimestamp: () => "ts" }, | |
| }); |
References
- Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)
| if (key === "length" || key === "name" || key === "prototype") { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
In strict mode, accessing or defining the "arguments" or "caller" properties on functions (especially bound functions, which are exotic objects) can throw a TypeError in some JavaScript engines. To ensure robustness and prevent potential runtime crashes, we should explicitly skip these properties when copying descriptors.
if (
key === "length" ||
key === "name" ||
key === "prototype" ||
key === "arguments" ||
key === "caller"
) {
continue;
}| try { | ||
| return JSON.stringify(err); | ||
| } catch { |
There was a problem hiding this comment.
JSON.stringify(err) can return undefined (not the string "undefined") if err is undefined, a function, or a symbol. Since getErrMsg is typed to return a string, returning undefined violates the type signature and can cause runtime errors in callers expecting a string. Using the nullish coalescing operator with String(err) ensures a string is always returned.
| try { | |
| return JSON.stringify(err); | |
| } catch { | |
| try { | |
| return JSON.stringify(err) ?? String(err); | |
| } catch { |
Description
Two Functions emulator bugs, both found while running a firebase-admin 13 project in the emulator. I found no existing issue for either.
1.
admin.firestore.Timestampandadmin.firestore.FieldValueareundefinedin the Functions emulator (firebase-admin 13).Proxied.getOriginalreturnsvalue.bind(target)for any function without aprototype.admin.firestoreas an arrow function with members attached (Timestamp,FieldValue, ...).Function.prototype.binddrops those own properties.The same code works in production and in plain Node, and fails only under the emulator. In practice, every Firestore write that builds a
Timestampthrough the namespace throws.2. A thrown non-
Errorvalue with a circular reference crashes the worker.getErrMsgcallsJSON.stringify(err), which throwsTypeError: Converting circular structure to JSONinside the emulator's rejection handler. The worker dies (socket hang up/ECONNRESET), and the real message is lost.Fix
Proxied.getOriginalstill binds, but now copies the function's own property descriptors (exceptlength,nameandprototype) onto the bound function, so statics and lazy getters survive.getErrMsgfalls back toutil.inspectwhenJSON.stringifythrows.Scenarios Tested
Repro (firebase-tools 15.34.2, firebase-admin 13, Node 22):
firebase emulators:start --only functions,firestore, then GETprobe: returns{"Timestamp":"undefined","FieldValue":"undefined"}before the fix and"function"for both after it (plain Node also gives"function").circ: before the fix, the worker dies withTypeError: Converting circular structure to JSON at getErrMsg. After it, the error is logged and the worker survives.src/emulator/functionsEmulatorRuntime.spec.ts(Proxiedis now exported for it): statics are kept, methods are still bound, and constructors and non-functions are untouched. The statics test fails without the fix (expected undefined to equal [Function: Timestamp]).src/error.spec.ts:getErrMsg/getErrStackon a circular value. It fails without the fix.npx mochaon both spec files: 7 passing. Prettier andtsc --noEmitare clean.Sample Commands
None. No commands or flags change.
🤖 Generated with Claude Code