Skip to content

fix(utils): Handle toJSON methods that return circular references #5323

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

Merged
merged 3 commits into from
Jun 28, 2022

Conversation

lforst
Copy link
Contributor

@lforst lforst commented Jun 28, 2022

This PR handles a case in normalize() when a .toJSON method on an input value either returns an object with a circular reference or the input value itself, causing problems down the line.

Changes

  • Normalize the return value from toJSON
  • Add tests to check for edge cases

Compromise

To detect circular references we need to do a memoize call before normalizing the return value from toJSON. However, at the location we're currently calling the toJSON method, value could still be a primitive like string or number and we can't call memoize with primitives because the underlying WeakSet only takes objects.

As a compromise, we're moving the toJSON call down to where value is certainly an object or array and after the circ-ref-check. I believe it is ok for us to only call toJSON on values that are non-primitives since you can't even set a field on strings, numbers, symbols, etc. unless you modify their prototype.

If somebody has a better idea on how to tackle this best, feel free to make suggestions! - I myself am not too happy with this solution.

@lforst lforst requested review from Lms24 and AbhiPrasad June 28, 2022 08:47
@github-actions
Copy link
Contributor

github-actions bot commented Jun 28, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.3 KB (+0.03% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 59.74 KB (+0.03% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 17.91 KB (+0.03% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 52.68 KB (+0.03% 🔺)
@sentry/browser - Webpack (gzipped + minified) 19.68 KB (+0.06% 🔺)
@sentry/browser - Webpack (minified) 64.06 KB (+0.04% 🔺)
@sentry/react - Webpack (gzipped + minified) 19.7 KB (+0.05% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified) 43.86 KB (+0.03% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 25.63 KB (+0.02% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 23.91 KB (+0.03% 🔺)

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

This LGTM. Left just two nits.
W.r.t only calling toJSON on objects/arrays and not on primitives: If we have tests that test normalization of primitives (which I think we do), I think this is fine.
(And if it turns out to cause problems, we can still revisit this)

Co-authored-by: Lukas Stracke <[email protected]>

Co-authored-by: Lukas Stracke <[email protected]>
@lforst lforst enabled auto-merge (squash) June 28, 2022 13:24
@lforst lforst disabled auto-merge June 28, 2022 14:04
@lforst lforst merged commit 893e157 into master Jun 28, 2022
@lforst lforst deleted the lforst-tojson-circ-refs-bug branch June 28, 2022 15:13
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