-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,28 +83,44 @@ | |
} | ||
|
||
/** | ||
* Remove any found empty strings, as they are dropped after round-tripping anyway | ||
* Execute `cb(obj, key)` on every object property where obj[key] is a string, recursively. | ||
* @param {any} obj | ||
* @param {(obj: Record<string, string>, key: string) => void} cb | ||
*/ | ||
function removeStrings(obj) { | ||
function iterateStrings(obj, cb) { | ||
if (obj && typeof obj === 'object' && !Array.isArray(obj)) { | ||
Object.keys(obj).forEach(key => { | ||
if (typeof obj[key] === 'string' && obj[key] === '') { | ||
delete obj[key]; | ||
} else if (typeof obj[key] === 'object' || Array.isArray(obj[key])) { | ||
removeStrings(obj[key]); | ||
if (typeof obj[key] === 'string') { | ||
cb(obj, key); | ||
} else { | ||
iterateStrings(obj[key], cb); | ||
} | ||
}); | ||
} else if (Array.isArray(obj)) { | ||
obj.forEach(item => { | ||
if (typeof item === 'object' || Array.isArray(item)) { | ||
removeStrings(item); | ||
iterateStrings(item, cb); | ||
} | ||
}); | ||
} | ||
} | ||
|
||
removeStrings(reportJson); | ||
iterateStrings(reportJson, (obj, key) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||
const value = obj[key]; | ||
|
||
// Remove empty strings, as they are dropped after round-tripping anyway. | ||
if (value === '') { | ||
delete obj[key]; | ||
return; | ||
} | ||
|
||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more. Could we throw an error if There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha, no error sounds good |
||
// @ts-expect-error node 20 | ||
obj[key] = value.toWellFormed(); | ||
} | ||
}); | ||
|
||
return reportJson; | ||
} | ||
|
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 ... 🪵 🤛