Skip to content

Conversation

@sinclairzx81
Copy link
Contributor

This PR should resolve issue #293. It carries out additional type === undefined checks on the additionalProperties: {} code path resolving to $asAny in these cases. This enables support for empty additionalProperties: {} schemas.

Notes

The check on line 543 was all that was necessary to resolve this issue. The preceding check on line 445 was added as $asAny seemed appropriate (both code paths are virtually identical), however I've not implemented any specific tests for this code path as I'm not certain what conditions trigger it. Feel free to remove if necessary.

Benchmark

FJS creation x 55,526 ops/sec ±0.32% (95 runs sampled)
CJS creation x 157,538 ops/sec ±0.36% (92 runs sampled)
JSON.stringify array x 3,709 ops/sec ±0.22% (94 runs sampled)
fast-json-stringify array x 8,293 ops/sec ±0.32% (95 runs sampled)
compile-json-stringify array x 7,437 ops/sec ±0.98% (88 runs sampled)
JSON.stringify long string x 10,064 ops/sec ±0.09% (98 runs sampled)
fast-json-stringify long string x 10,056 ops/sec ±0.12% (98 runs sampled)
compile-json-stringify long string x 10,093 ops/sec ±0.08% (96 runs sampled)
JSON.stringify short string x 9,359,647 ops/sec ±0.30% (94 runs sampled)
fast-json-stringify short string x 52,095,769 ops/sec ±0.29% (97 runs sampled)
compile-json-stringify short string x 38,945,526 ops/sec ±0.22% (96 runs sampled)
JSON.stringify obj x 1,918,121 ops/sec ±0.15% (95 runs sampled)
fast-json-stringify obj x 8,553,618 ops/sec ±0.36% (93 runs sampled)
compile-json-stringify obj x 20,126,548 ops/sec ±1.39% (92 runs sampled)

Checklist

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

@mcollina mcollina merged commit 8b3d849 into fastify:master Mar 8, 2021
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.

2 participants