Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ads BigInt support #48

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Ads BigInt support #48

wants to merge 3 commits into from

Conversation

pixtron
Copy link

@pixtron pixtron commented Jan 23, 2020 •
edited
Loading

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 and deterministicJsonSafe.

Resolves: #41

Had an earlier version which replaced all JSON.stringify calls, with a bigintSafeJSONStringify 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 former decirc method, hence the rename to jsonSafe as it does more then decirculate now.

Performance is now about the same (sometimes faster, sometimes slower) as in master.

function bigintSafeJSONStringify (obj, replacer, spacer) {
  return JSON.stringify(obj, function (key, value) {
    replacer = replacer !== undefined ? replacer : function (k, v) { return v }
    return replacer(key, typeof value === 'bigint' ? value.toString() : value)
  }, spacer)
}

Also i moved quite a bit of code from the former decirc method to a new replaceProperty method to avoid code duplication.

@pixtron pixtron requested a review from mcollina January 23, 2020 03:45
mcollina
mcollina approved these changes Jan 23, 2020
View reviewed changes
Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Collaborator

Can you post the benchmark results?

@pixtron
Copy link
Author

pixtron commented Jan 23, 2020

This are the benchmark results on my MBP with 2.9 GHz Intel Core i7 using node.js 10.4.1

This PR

util.inspect:          simple object                  x 328,495 ops/sec ±1.43% (84 runs sampled)
util.inspect:          circular                       x 144,068 ops/sec ±1.85% (88 runs sampled)
util.inspect:          circular getters               x 142,615 ops/sec ±1.26% (86 runs sampled)
util.inspect:          deep                           x 9,866 ops/sec ±0.95% (90 runs sampled)
util.inspect:          deep circular                  x 9,913 ops/sec ±0.91% (90 runs sampled)
util.inspect:          large deep circular getters    x 7,170 ops/sec ±1.23% (92 runs sampled)
util.inspect:          deep non-conf circular getters x 10,043 ops/sec ±1.37% (93 runs sampled)

json-stringify-safe:   simple object                  x 282,385 ops/sec ±1.36% (93 runs sampled)
json-stringify-safe:   circular                       x 134,416 ops/sec ±1.08% (94 runs sampled)
json-stringify-safe:   circular getters               x 136,655 ops/sec ±0.42% (92 runs sampled)
json-stringify-safe:   deep                           x 9,452 ops/sec ±3.60% (85 runs sampled)
json-stringify-safe:   deep circular                  x 9,581 ops/sec ±1.13% (93 runs sampled)
json-stringify-safe:   large deep circular getters    x 366 ops/sec ±1.58% (87 runs sampled)
json-stringify-safe:   deep non-conf circular getters x 9,267 ops/sec ±1.18% (88 runs sampled)

fast-safe-stringify:   simple object                  x 1,136,994 ops/sec ±0.62% (88 runs sampled)
fast-safe-stringify:   circular                       x 561,510 ops/sec ±1.04% (91 runs sampled)
fast-safe-stringify:   circular getters               x 572,254 ops/sec ±0.75% (91 runs sampled)
fast-safe-stringify:   deep                           x 33,181 ops/sec ±1.09% (91 runs sampled)
fast-safe-stringify:   deep circular                  x 32,171 ops/sec ±1.20% (90 runs sampled)
fast-safe-stringify:   large deep circular getters    x 1,302 ops/sec ±0.52% (90 runs sampled)
fast-safe-stringify:   deep non-conf circular getters x 16,366 ops/sec ±0.53% (93 runs sampled)

Current master (0e011f0)

util.inspect:          simple object                  x 311,946 ops/sec ±2.71% (87 runs sampled)
util.inspect:          circular                       x 131,329 ops/sec ±3.60% (84 runs sampled)
util.inspect:          circular getters               x 139,632 ops/sec ±1.82% (87 runs sampled)
util.inspect:          deep                           x 9,756 ops/sec ±0.88% (89 runs sampled)
util.inspect:          deep circular                  x 9,654 ops/sec ±0.85% (91 runs sampled)
util.inspect:          large deep circular getters    x 6,845 ops/sec ±1.39% (89 runs sampled)
util.inspect:          deep non-conf circular getters x 9,642 ops/sec ±1.31% (92 runs sampled)

json-stringify-safe:   simple object                  x 269,690 ops/sec ±0.49% (94 runs sampled)
json-stringify-safe:   circular                       x 128,472 ops/sec ±1.10% (88 runs sampled)
json-stringify-safe:   circular getters               x 127,650 ops/sec ±1.64% (90 runs sampled)
json-stringify-safe:   deep                           x 10,017 ops/sec ±0.98% (93 runs sampled)
json-stringify-safe:   deep circular                  x 9,785 ops/sec ±1.22% (92 runs sampled)
json-stringify-safe:   large deep circular getters    x 365 ops/sec ±1.27% (83 runs sampled)
json-stringify-safe:   deep non-conf circular getters x 9,498 ops/sec ±1.21% (92 runs sampled)

fast-safe-stringify:   simple object                  x 1,030,311 ops/sec ±3.73% (84 runs sampled)
fast-safe-stringify:   circular                       x 566,608 ops/sec ±1.00% (90 runs sampled)
fast-safe-stringify:   circular getters               x 577,558 ops/sec ±1.08% (90 runs sampled)
fast-safe-stringify:   deep                           x 33,749 ops/sec ±1.07% (94 runs sampled)
fast-safe-stringify:   deep circular                  x 33,269 ops/sec ±0.52% (93 runs sampled)
fast-safe-stringify:   large deep circular getters    x 1,310 ops/sec ±0.57% (93 runs sampled)
fast-safe-stringify:   deep non-conf circular getters x 15,826 ops/sec ±2.21% (89 runs sampled)

@pixtron
Copy link
Author

pixtron commented Jan 23, 2020

This are the benchmark results on my MBP with 2.9 GHz Intel Core i7 using node.js 12.13.1

This PR

util.inspect:          simple object                  x 273,452 ops/sec ±0.54% (92 runs sampled)
util.inspect:          circular                       x 124,780 ops/sec ±4.47% (88 runs sampled)
util.inspect:          circular getters               x 131,142 ops/sec ±0.85% (91 runs sampled)
util.inspect:          deep                           x 8,991 ops/sec ±1.74% (91 runs sampled)
util.inspect:          deep circular                  x 8,952 ops/sec ±1.06% (92 runs sampled)
util.inspect:          large deep circular getters    x 6,471 ops/sec ±1.86% (91 runs sampled)
util.inspect:          deep non-conf circular getters x 9,427 ops/sec ±0.63% (91 runs sampled)

json-stringify-safe:   simple object                  x 267,693 ops/sec ±1.65% (89 runs sampled)
json-stringify-safe:   circular                       x 125,240 ops/sec ±3.69% (88 runs sampled)
json-stringify-safe:   circular getters               x 129,678 ops/sec ±1.28% (96 runs sampled)
json-stringify-safe:   deep                           x 10,329 ops/sec ±1.90% (92 runs sampled)
json-stringify-safe:   deep circular                  x 10,123 ops/sec ±2.01% (92 runs sampled)
json-stringify-safe:   large deep circular getters    x 399 ops/sec ±0.52% (91 runs sampled)
json-stringify-safe:   deep non-conf circular getters x 9,900 ops/sec ±0.52% (93 runs sampled)

fast-safe-stringify:   simple object                  x 1,392,552 ops/sec ±0.93% (91 runs sampled)
fast-safe-stringify:   circular                       x 583,394 ops/sec ±1.03% (94 runs sampled)
fast-safe-stringify:   circular getters               x 581,332 ops/sec ±1.05% (93 runs sampled)
fast-safe-stringify:   deep                           x 33,786 ops/sec ±0.83% (93 runs sampled)
fast-safe-stringify:   deep circular                  x 29,492 ops/sec ±4.71% (85 runs sampled)
fast-safe-stringify:   large deep circular getters    x 1,267 ops/sec ±1.31% (92 runs sampled)
fast-safe-stringify:   deep non-conf circular getters x 15,113 ops/sec ±1.51% (89 runs sampled)

Current master (0e011f0)

util.inspect:          simple object                  x 253,229 ops/sec ±0.42% (90 runs sampled)
util.inspect:          circular                       x 123,059 ops/sec ±2.30% (90 runs sampled)
util.inspect:          circular getters               x 123,586 ops/sec ±1.13% (88 runs sampled)
util.inspect:          deep                           x 8,930 ops/sec ±0.71% (91 runs sampled)
util.inspect:          deep circular                  x 8,493 ops/sec ±1.96% (89 runs sampled)
util.inspect:          large deep circular getters    x 5,057 ops/sec ±0.35% (93 runs sampled)
util.inspect:          deep non-conf circular getters x 8,780 ops/sec ±1.75% (91 runs sampled)

json-stringify-safe:   simple object                  x 271,285 ops/sec ±1.15% (94 runs sampled)
json-stringify-safe:   circular                       x 121,078 ops/sec ±2.45% (90 runs sampled)
json-stringify-safe:   circular getters               x 115,020 ops/sec ±2.40% (85 runs sampled)
json-stringify-safe:   deep                           x 9,312 ops/sec ±1.22% (88 runs sampled)
json-stringify-safe:   deep circular                  x 8,426 ops/sec ±1.43% (82 runs sampled)
json-stringify-safe:   large deep circular getters    x 324 ops/sec ±1.49% (79 runs sampled)
json-stringify-safe:   deep non-conf circular getters x 8,194 ops/sec ±0.91% (89 runs sampled)

fast-safe-stringify:   simple object                  x 1,156,823 ops/sec ±1.45% (91 runs sampled)
fast-safe-stringify:   circular                       x 561,062 ops/sec ±1.45% (87 runs sampled)
fast-safe-stringify:   circular getters               x 572,426 ops/sec ±0.63% (90 runs sampled)
fast-safe-stringify:   deep                           x 32,251 ops/sec ±0.53% (94 runs sampled)
fast-safe-stringify:   deep circular                  x 30,839 ops/sec ±0.70% (94 runs sampled)
fast-safe-stringify:   large deep circular getters    x 1,164 ops/sec ±6.97% (84 runs sampled)
fast-safe-stringify:   deep non-conf circular getters x 13,397 ops/sec ±1.10% (88 runs sampled)

@mcollina
Copy link
Collaborator

Seems good, thanks!

@CupNoodleFork
Copy link

I need this feature, when to release?

@davidmarkclements
Copy link
Owner

davidmarkclements commented May 24, 2020 •
edited
Loading

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

@karlbohlmark
Copy link

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.

@pixtron
Copy link
Author

pixtron commented Dec 3, 2020

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".
Thats why i created this PR, to have a "safe" alternative to JSON.stringify.

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.
Although eventually it would make sense to append 'n' or '#bigint' to the BigInt strings, so it would be possible to parse those detect those strings in a reviver function in JSON.parse and parse them to BigInt's.

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.

@titanism titanism mentioned this pull request Dec 6, 2022
Closed
@titanism
Copy link

titanism commented Dec 6, 2022

One could argue that you've already deviated far enough from the default behavior of JSON.stringify by having [Circular] added as keys. Therefore you should not stop there and simply support the two other odd behaviors we need to account for, which are BigInt's and Symbol's, with the exact same behavior, e.g. wrapping with [] brackets.

@gustawdaniel
Copy link

gustawdaniel commented Sep 6, 2023 •
edited
Loading

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 (https://github.com/lukeed/uid) where a tradeoff selection was left for the user on the level of import. For example in uid it is solved by:

uid
The default is "non-secure", which uses Math.random to produce UUIDs.
uid/secure
The "secure" mode produces cryptographically secure (CSPRNG) UUIDs using the current environment's crypto module.
uid/single
The "single" mode does not maintain an internal cache, which makes it ideal for short-lived environments.

@titanism
Copy link

@gustawdaniel the package superjson does not export CJS and is strictly ESM, so this is not a drop-in alternative to fast-safe-stringify

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@BridgeAR BridgeAR Awaiting requested review from BridgeAR

@davidmarkclements davidmarkclements Awaiting requested review from davidmarkclements

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for BigInt
7 participants
@pixtron @mcollina @CupNoodleFork @davidmarkclements @karlbohlmark @titanism @gustawdaniel