-
-
Notifications
You must be signed in to change notification settings - Fork 209
well formed unicode string (#147) #151
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
well formed unicode string (#147) #151
Conversation
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.
How much does this impact our benchmarks?
Overall this sounds correct, even if we don’t have the fixed V8 yet.
In $asStringSmall there is one for that loops over input string. I have put surrogate detector condition in this loop. So I think it will has no impact on benchmarks. |
Just run https://github.com/fastify/fast-json-stringify/blob/master/bench.js before and after your change. |
befor: |
after: |
You were likely running something else on your machines, as in the “after” all JSON.stringify benchmarks went down ad well. Can you rerun? |
@@ -236,16 +236,22 @@ function $asString (str) { | |||
// magically escape strings for json | |||
// relying on their charCodeAt | |||
// everything below 32 needs JSON.stringify() | |||
// every string that contain surrogate needs JSON.stringify() |
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 not accurate. Every lone surrogate needs escaping. Surrogates that appear in valid surrogate pairs must not be escaped.
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 know @mathiasbynens
I read your Implementation In c. But I think implementing it in JavaScript would be heavy.
The approach of @mcollina is an heuristic approach not deterministic.
Using of surrogate in keys of object in js is not common.
So I decided to use standard stringify when we encounter such character
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 we should clarify that comment and write down why we use that heuristics: doing the full algorithm here would be too expensive. However the penalty to try this for lower length strings and falling back to JSON.stringify is ok.
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.
FWIW, I think this approach makes total sense. Lone surrogates are an edge case after all.
Hi @mcollina |
test/surrogate.test.js
Outdated
const validate = validator(schema) | ||
const stringify = build(schema) | ||
const output = stringify('\uDF06\uD834') | ||
// t.equal(output, '"\\udf06\\ud834"') |
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.
It would be great to use a mirror test for this.
t.equal(output, JSON.stringify('\uDF06\uD834'))
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.
What do you mean by mirror test?(Excuse me if this question is simple :)) )
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.
What I put above. Basically you have a target system A. You want to verify that B calls A and returns that output. You call B() and then A() and you compare results. In this way any change of output in A() will be automatically reflected in B() output.
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.
Thanks. I got it.
It seems we are losing a bit:
~/repositories/fast-json-stringify (master)
|
I changed $asStringSmall function so It can detect surrogate character.
But in surrogate.test.js if you uncomment line 50 and 65 , test will fail.
I am using node 11.12.0.
Is JSON.Stringify() is up to date or what @mathiasbynens has said in issue 147 is in test phase?