-
Notifications
You must be signed in to change notification settings - Fork 27
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
Ads BigInt support #48
base: master
Are you sure you want to change the base?
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.
lgtm
Can you post the benchmark results? |
This are the benchmark results on my MBP with 2.9 GHz Intel Core i7 using node.js 10.4.1 This PR
Current master (0e011f0)
|
This are the benchmark results on my MBP with 2.9 GHz Intel Core i7 using node.js 12.13.1 This PR
Current master (0e011f0)
|
Seems good, thanks! |
I need this feature, when to release? |
I'm not sure about this, do we want to support BigInt if JSON.stringify doesn't? I don't think we should do this until JSON.stringify is updated, and then we should follow that format. Converting BigInt to a string is better than a number because it keeps resolution, but when it's parsed on the other side you still need to be able to handle that. So I think handling this case should actually live in userland for now (since it needs to live in userland for parse anyway) I'm open to changing my mind on this however if a strong counterpoint is made |
I hear you about not wanting to support BigInt if JSON.stringify doesn't support it and if you don't have a need for it personally, I understand if you want to leave it out. For me as a user however, it makes sense to support BigInt the same way that it makes sense to support circular structures. My reason to use this library instead of JSON.stringify is that I want a "safe" alternative that can handle any object without throwing. |
I understand you, not wanting to support BigInt, as long as JSON.stringify and JSON.parse do not support it. To me the "safe" in fast-safe-stringify relates to "Gracefully handles ... instead of throwing". I see your point regarding not being able to parse the serialized BigInt, but neither does parsing an object with circular structures that has been serialized by fss restore the circular structure. If you think handling of BigInt should live in userland, feel free to close this PR. Handling of BigInt can indeed be achieved in userland with a replacer function. |
One could argue that you've already deviated far enough from the default behavior of JSON.stringify by having |
For people that reading this PR and looking for Stringify that supports Bigint https://github.com/blitz-js/superjson can be interesting. In the case of this library, we can apply the approach from uid |
@gustawdaniel the package |
Beside adding BigInt support, this PR also ads node 12 to .travis.yml and updates standard to
^14.0.0
Not really sure about the new method names
jsonSafe
anddeterministicJsonSafe
.Resolves: #41
Had an earlier version which replaced all
JSON.stringify
calls, with abigintSafeJSONStringify
method (see below). But this had huge impacts on the performance (only about a third ops/sec than before). Therefore i decided to implement the toString conversion for BigInts directly in the formerdecirc
method, hence the rename tojsonSafe
as it does more then decirculate now.Performance is now about the same (sometimes faster, sometimes slower) as in master.
Also i moved quite a bit of code from the former
decirc
method to a newreplaceProperty
method to avoid code duplication.