Skip to content

fix: Improve logging of errors in the browser if a circular reference or bigint is encountered #9181

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MarkDuckworth
Copy link
Contributor

@MarkDuckworth MarkDuckworth commented Jul 23, 2025

An alternative solution to get information for #8903.

As reported by the user in #8093, err.name and err.message were undefined after the fix of #8907 was applied. Based on the previous behavior it's likely that error was not actually an error object, but instead it was some object that was failing to stringify with JSON.stringify. This solution reverts getting err.name and instead attempts to work around possible JSON.stringify issues.

Copy link

changeset-bot bot commented Jul 23, 2025

⚠️ No Changeset found

Latest commit: 0179f79

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@MarkDuckworth MarkDuckworth marked this pull request as ready for review July 23, 2025 23:00
@MarkDuckworth MarkDuckworth requested review from a team as code owners July 23, 2025 23:00
@ehsannas ehsannas self-requested a review July 23, 2025 23:07
@google-oss-bot
Copy link
Contributor

Size Report 1

Affected Products

  • @firebase/firestore

    TypeBase (a4897a6)Merge (241e0e5)Diff
    browser391 kB392 kB+236 B (+0.1%)
    module391 kB392 kB+236 B (+0.1%)
    react-native392 kB392 kB+236 B (+0.1%)
  • @firebase/firestore-lite

    TypeBase (a4897a6)Merge (241e0e5)Diff
    browser115 kB116 kB+262 B (+0.2%)
    module115 kB116 kB+262 B (+0.2%)
    react-native116 kB116 kB+262 B (+0.2%)
  • bundle

    15 size changes

    TypeBase (a4897a6)Merge (241e0e5)Diff
    firestore (CSI Auto Indexing Disable and Delete)287 kB287 kB+197 B (+0.1%)
    firestore (CSI Auto Indexing Enable)287 kB287 kB+197 B (+0.1%)
    firestore (Persistence)319 kB319 kB+197 B (+0.1%)
    firestore (Query Cursors)258 kB258 kB+197 B (+0.1%)
    firestore (Query)256 kB256 kB+197 B (+0.1%)
    firestore (Read data once)246 kB246 kB+197 B (+0.1%)
    firestore (Read Write w Persistence)339 kB339 kB+197 B (+0.1%)
    firestore (Realtime updates)246 kB246 kB+197 B (+0.1%)
    firestore (Transaction)224 kB225 kB+197 B (+0.1%)
    firestore (Write data)226 kB226 kB+197 B (+0.1%)
    firestore-lite (Query Cursors)110 kB110 kB+223 B (+0.2%)
    firestore-lite (Query)106 kB106 kB+223 B (+0.2%)
    firestore-lite (Read data once)81.5 kB81.7 kB+223 B (+0.3%)
    firestore-lite (Transaction)107 kB107 kB+223 B (+0.2%)
    firestore-lite (Write data)91.0 kB91.2 kB+223 B (+0.2%)

  • firebase

    TypeBase (a4897a6)Merge (241e0e5)Diff
    firebase-compat.js800 kB800 kB+186 B (+0.0%)
    firebase-firestore-compat.js349 kB349 kB+186 B (+0.1%)
    firebase-firestore-lite.js138 kB139 kB+242 B (+0.2%)
    firebase-firestore.js455 kB455 kB+216 B (+0.0%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/DFPq5S7og1.html

@google-oss-bot
Copy link
Contributor

Size Analysis Report 1

This report is too large (638,603 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/cymZSXwZiF.html

@dconeybe
Copy link
Contributor

Drive-by comment: Are you able to add unit test coverage?

Copy link
Contributor

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

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

+1 to adding a unit test that invokes formatJSON with object containing the two special cases. Looks good otherwise.

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.

4 participants