Skip to content

Conversation

@MetalMichael
Copy link
Contributor

Fixes #299

Basically JSON.stringify doesn't work for Functions and Symbols, and results in undefined, which is invalid JSON: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#description

So instead, when dealing with additionalProperties, make sure we aren't trying to serialize symbols or functions.

Checklist

[13:26] (fix-serializing-undefined) fast-json-stringify $ yarn run test
yarn run v1.22.4
$ npm run test:lint && npm run test:unit && npm run test:typescript

> [email protected] test:lint repos/fast-json-stringify
> standard
> [email protected] test:unit repos/fast-json-stringify
> tap -J test/*.test.js test/**/*.test.js

 PASS  test/const.test.js 6 OK 173.52ms
 PASS  test/clean-cache.test.js 2 OK 67.577ms
 PASS  test/basic.test.js 67 OK 242.574ms
 PASS  test/array.test.js 24 OK 250.685ms
 PASS  test/anyof.test.js 25 OK 353.127ms
 PASS  test/any.test.js 25 OK 124.641ms
 PASS  test/allof.test.js 11 OK 39.659ms
 PASS  test/additionalProperties.test.js 18 OK 216.726ms
 PASS  test/debug-mode.test.js 8 OK 218.186ms
 PASS  test/inferType.test.js 15 OK 105.097ms
 PASS  test/date.test.js 40 OK 361.89ms
 PASS  test/defaults.test.js 12 OK 151.718ms
 PASS  test/long.test.js 8 OK 143.049ms
 PASS  test/integer.test.js 34 OK 136.064ms
 PASS  test/invalidSchema.test.js 2 OK 25.13ms
 PASS  test/if-then-else.test.js 10 OK 567.512ms
 PASS  test/nestedObjects.test.js 1 OK 16.34ms
 PASS  test/missing-values.test.js 7 OK 152.738ms
 PASS  test/nullable.test.js 8 OK 76.919ms
 PASS  test/patternProperties.test.js 7 OK 89.467ms
 PASS  test/regex.test.js 3 OK 90.434ms
 PASS  test/sanitize.test.js 3 OK 7.231ms
 PASS  test/sanitize2.test.js 1 OK 4.405ms
 PASS  test/ref.test.js 52 OK 496.434ms
 PASS  test/oneof.test.js 20 OK 683.684ms
 PASS  test/required.test.js 23 OK 61.754ms
 PASS  test/sanitize6.test.js 1 OK 4.081ms
 PASS  test/sanitize4.test.js 1 OK 6.357ms
 PASS  test/sanitize3.test.js 1 OK 6.06ms
 PASS  test/sanitize5.test.js 1 OK 8.566ms
 PASS  test/surrogate.test.js 8 OK 108.537ms
 PASS  test/toJSON.test.js 7 OK 117.36ms
 PASS  test/side-effect.test.js 7 OK 314.225ms
 PASS  test/unknownFormats.test.js 1 OK 19.633ms
 PASS  test/json-schema-test-suite/draft4.test.js 3 OK 48.62ms
 PASS  test/json-schema-test-suite/draft6.test.js 4 OK 104.46ms
 PASS  test/typesArray.test.js 27 OK 245.222ms
 PASS  test/json-schema-test-suite/draft7.test.js 4 OK 71.275ms

                         
  🌈 SUMMARY RESULTS 🌈  
                         

Suites:   38 passed, 38 of 38 completed
Asserts:  497 passed, of 497
Time:     5s

> [email protected] test:typescript repos/fast-json-stringify
> tsc --project ./test/types/tsconfig.json

✨  Done in 10.00s.

[13:27] (fix-serializing-undefined) fast-json-stringify $ yarn run benchmark
yarn run v1.22.4
$ node bench.js
FJS creation x 47,784 ops/sec ±5.00% (86 runs sampled)
CJS creation x 153,326 ops/sec ±1.56% (90 runs sampled)
JSON.stringify array x 4,545 ops/sec ±0.42% (93 runs sampled)
fast-json-stringify array x 6,045 ops/sec ±0.68% (91 runs sampled)
compile-json-stringify array x 5,961 ops/sec ±0.70% (90 runs sampled)
JSON.stringify long string x 11,318 ops/sec ±0.38% (97 runs sampled)
fast-json-stringify long string x 11,163 ops/sec ±0.70% (93 runs sampled)
compile-json-stringify long string x 11,347 ops/sec ±0.41% (95 runs sampled)
JSON.stringify short string x 8,854,949 ops/sec ±0.48% (94 runs sampled)
fast-json-stringify short string x 27,650,889 ops/sec ±2.32% (90 runs sampled)
compile-json-stringify short string x 28,937,678 ops/sec ±0.44% (91 runs sampled)
JSON.stringify obj x 2,080,972 ops/sec ±0.57% (89 runs sampled)
fast-json-stringify obj x 11,239,549 ops/sec ±0.96% (94 runs sampled)
compile-json-stringify obj x 14,636,055 ops/sec ±0.64% (94 runs sampled)
✨  Done in 76.33s.
[13:28] (fix-serializing-undefined) fast-json-stringify $ 

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AdditionalProperties with method references causes invalid JSON "undefined"

4 participants