Skip to content
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

Fix error when serializing objects with circular referencing #438

Merged
merged 6 commits into from
Aug 10, 2023

Conversation

mlostekk
Copy link
Contributor

@mlostekk mlostekk commented Aug 8, 2023

When trying to serialize an object like this

const subObject: { sub1: string, sub2: any } = { sub1: 'subObjectValue', sub2: undefined }
const givenObject = { root1: 'some value', root2: subObject };
subObject.sub2 = givenObject;

one will get an error like this

    TypeError: Converting circular structure to JSON
        --> starting at object with constructor 'Object'
        |     property 'b' -> object with constructor 'Object'
        --- property 'd' closes the circle
        at JSON.stringify (<anonymous>)

This PR fixes this behavior. A "flattened" output can be seen in the added unit test.

@lucas-zimerman
Copy link
Collaborator

Hey thank you for opening the PR!
Sorry to ask for this change, but since recently this was addressed on React Native, could you also use the way it was solved on React Native?

Basically copy the normalise to util/normalise
https://github.com/getsentry/sentry-react-native/blob/9ee4637908664fd8c5c34d592589352f6112e102/src/js/utils/normalize.ts
and instead of calling JSON.stringify(value);

Additionally, we could also add the tests that were used here:
https://github.com/getsentry/sentry-react-native/blob/9ee4637908664fd8c5c34d592589352f6112e102/test/utils/normalize.test.ts

@mlostekk
Copy link
Contributor Author

mlostekk commented Aug 9, 2023

@lucas-zimerman Not entirely sure if i covered all areas. But I have updated it accordingly to the RN solution

Copy link
Collaborator

@lucas-zimerman lucas-zimerman left a comment

Choose a reason for hiding this comment

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

looks to to me, thank you for the contribution!

@lucas-zimerman lucas-zimerman merged commit fb68723 into getsentry:main Aug 10, 2023
4 of 5 checks passed
lucas-zimerman added a commit that referenced this pull request Aug 10, 2023
This PR only adds the missing changelog from #438 to the changelog.md file.
lucas-zimerman added a commit that referenced this pull request Aug 10, 2023
* Update CHANGELOG.md

This PR only adds the missing changelog from #438 to the changelog.md file.

* Update CHANGELOG.md
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.

2 participants