-
Notifications
You must be signed in to change notification settings - Fork 887
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
perf: optimize json string escaping #1717
base: main
Are you sure you want to change the base?
perf: optimize json string escaping #1717
Conversation
// eslint-disable-next-line | ||
const STR_ESCAPE = /[\u0000-\u001f\u0022\u005c\ud800-\udfff]|[\ud800-\udbff](?![\udc00-\udfff])|(?:[^\ud800-\udbff]|^)[\udc00-\udfff]/ | ||
|
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.
I think this is very mysterious (i.e. I have no clue what it is looking for and don't want to expend the mental gymnastics to figure it out). If we are going to introduce complicated regular expressions, I'd rather see https://www.npmjs.com/package/xregexp used to make them legible.
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 contains the groups of chars that should be escaped. Not sure if this can be not mysterious.
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.
Roughly:
https://runkit.com/embed/rx8essijm0uh
var xregexp = require("xregexp")
const STR_ESCAPE = xregexp(
`[\u0000-\u001f\u0022\u005c\ud800-\udfff] # chars A - G
| [\ud800-\udbff] # chars H - J
(?![\udc00-\udfff]) # BUT NOT something else
| (?:[^\ud800-\udbff]|^) # some explantation
[\udc00-\udfff] # something else
`
,
'x'
)
It's clearer, but could probably be even more clear using XRegExp.build
.
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.
A jsdoc block that explains each part would also suffice (and keep a concerning dependency tree from being added).
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.
lgtm
Similar PR for the same escaping function.
fastify/fast-json-stringify#553