-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
misc(proto): ensure all strings are well-formed #15909
Conversation
} | ||
}); | ||
} | ||
} | ||
|
||
removeStrings(reportJson); | ||
iterateStrings(reportJson, (obj, key) => { |
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.
rest of patch lgtm, but maybe extract this cb into a fn with a name? removeEmptyAndDropLoneSurrogates
?
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.
Aren't the comments inside sufficient and the code readable? detaching this anonymous fn from the utility function is more jumping around and doesn't help readabilty imo.
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.
well i suppose this entire file is also in the vein of "read it if you want to know what it does" so.. whatevs
|
||
// Sanitize lone surrogates. | ||
// @ts-expect-error node 20 | ||
if (String.prototype.isWellFormed && !value.isWellFormed()) { |
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.
Could we throw an error if String.prototype.isWellFormed
isn't available? Seems like we would want to fail loudly in that case.
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.
We absolutely do not want to throw an exception here, that would take down PSI for a very minor reason. Anyhow, WRS ships a Chrome that supports this and won't ever regress on that front.
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.
Gotcha, no error sounds good
} | ||
}); | ||
} else if (Array.isArray(obj)) { | ||
obj.forEach(item => { | ||
if (typeof item === 'object' || Array.isArray(item)) { | ||
removeStrings(item); | ||
iterateStrings(item, cb); |
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.
Technically this allows arrays with non-well-formed strings in them to pass through unchanged.
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.
ah... same is true for empty strings sticking around. maybe it's fine ... 🪵 🤛
fixes #15908
ref #14911