Skip to content

Conversation

@bitekas
Copy link
Contributor

@bitekas bitekas commented Jun 8, 2025

Description

Fixing the issue #1703.

Googleapis returns a proxy to an object with a missing "size" property due to an outdated node-fetch library, and this causes the logger to throw an exception. This PR fixes that.

Also improved lookup time to O(1), and fixed a potential bug for logging objects such as { toJSON: true }: in the current implementation debug({toJSON: true}); is throwing an exception. Pretty nasty given the amount of the Internet using this logger, and hard to triage. This is also fixed.

Code sample

@CorieW CorieW linked an issue Jun 10, 2025 that may be closed by this pull request
@CorieW
Copy link
Member

CorieW commented Jun 10, 2025

These changes seem to work well.

Before I see about getting this reviewed and merged in, would be able to fix the linting problem?

Thanks for your contribution @bitekas.

@bitekas
Copy link
Contributor Author

bitekas commented Jun 11, 2025

@CorieW the linter errors are fixed.

FYI, here's the pull request to resolve the underlying android publisher problem: node-fetch/node-fetch#1849, they are linked.

@cabljac cabljac requested review from cabljac and Copilot June 16, 2025 08:36
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes logger runtime exceptions by addressing issues with circular reference handling and improving lookup efficiency.

  • Refactored circular reference detection to utilize a Set instead of an array for O(1) lookup.
  • Updated property iteration logic to better handle errors and inherited properties.
Comments suppressed due to low confidence (2)

src/logger/index.ts:76

  • The try-catch block here may obscure underlying serialization errors that could be useful for debugging. Consider logging the error or handling it in a way that preserves useful error context.
try { if (refs.has(obj[k])) { returnObj[k] = "[Circular]"; } else { returnObj[k] = removeCircular(obj[k], refs); } } catch { returnObj[k] = "[Error - cannot serialize]"; }

src/logger/index.ts:85

  • [nitpick] The fallback for inherited properties may introduce unexpected serialization output. Consider reviewing whether prototype properties should be serialized or omitted entirely.
else { returnObj[k] = "[Error - defined in the prototype but missing in the object]"; }

Copy link
Contributor

@cabljac cabljac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@cabljac cabljac requested review from taeold and removed request for taeold July 9, 2025 10:19
@taeold
Copy link
Contributor

taeold commented Jul 11, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces fixes and improvements to the logging serialization logic. The change to use a Set for circular reference detection is a performance enhancement. The fixes for objects with a non-function toJSON property and for properties that throw on access make the logger more robust. The handling of prototype properties could lead to incorrect serialization for objects using inheritance, and a change has been suggested to align with standard serializer behavior by ignoring these properties.

@taeold taeold added this pull request to the merge queue Oct 8, 2025
Merged via the queue into firebase:master with commit 539639d Oct 8, 2025
8 checks passed
@github-project-automation github-project-automation bot moved this from Approved [PR] to Done in [Cloud] Extensions + Functions Oct 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Logger: unhandled runtime exceptions

4 participants