-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
ref(utils): Simplify normalization code and be more specific when erroring #4761
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
lobsterkatie
merged 7 commits into
master
from
kmclb-specify-which-property-is-unserializable
Mar 25, 2022
Merged
ref(utils): Simplify normalization code and be more specific when erroring #4761
lobsterkatie
merged 7 commits into
master
from
kmclb-specify-which-property-is-unserializable
Mar 25, 2022
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
size-limit report
|
0e574e6
to
84e9545
Compare
cc @timfish |
lforst
reviewed
Mar 24, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great PR. It simplifies a lot! I left some thoughts (and maybe a bug?) below.
Looks like @lforst has already give this a comprehensive review but looks like a good improvement! |
84e9545
to
760ca73
Compare
size-limit report 📦
|
760ca73
to
c8f8cf6
Compare
lforst
approved these changes
Mar 25, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, if an error occurs anywhere in the normalization process, the entire value is clobbered, in favor of the string
"**non-serializable**"
. This has a few drawbacks:normalize
an object and get back a string, which can prove problematic if data is later added to the normalized property as if it were still an object (see the issue linked below).The goal of this PR, then, was to identify all of the places where errors could occur and catch them right there, rather than relying on the
try-catch
in the mainnormalize
function. In order to make this easier to reason about, the normalization code was first streamlined a bit. Key changes:serializeValue
andmakeSerializable
have been combined into a single function,stringifyValue
, and any code not resulting in a string has been moved into the calling function.stringifyValue
has been wrapped in atry-catch
, so that errors which originate there will only affect the value at hand, rather than then entire object."**non-serializable**"
has to be used, it's now accompanied by the message belonging to the error which broke the serialization.normalize
function needs to return"**non-serializable**"
, it now wraps it in an object, so that other data can still be added.walk
function has been renamedvisit
, to better conform to the language of the Visitor Pattern. For the moment, it's still aliased towalk
when exported, to preserve backwards compatibility.getWalkSource
function has been renamedconvertToPlainObject
, to better reflect what it does, and some of its repetitive code has been extracted into theserializeEventTarget
andgetOwnProperties
functions.Fixes #4719
Ref: https://getsentry.atlassian.net/browse/WEB-703