Skip to content

Ajv serializer#45

Merged
Eomm merged 8 commits intomainfrom
ajv-serializer
Oct 31, 2022
Merged

Ajv serializer#45
Eomm merged 8 commits intomainfrom
ajv-serializer

Conversation

@Eomm
Copy link
Copy Markdown
Member

@Eomm Eomm commented Dec 6, 2021

Opening this draft to get feedback.
Right now this feature works, it needs more testing (CI fails due to the coverage).

The notable code is on the index and serializer-compiler files, the validator is just a copy-paste

AJV-JTD can be used to serialize the JSON payloads: https://ajv.js.org/guide/getting-started.html#parsing-and-serializing-json

I'm not sure if it is good to include this feature in this package, or we should create a new package that:

  • support fast-json-stringify by default
  • let the user opt-in to ajv-JTD feature

What do you think?

Closes fastify/fastify#3441

@Eomm Eomm requested a review from a team December 9, 2021 14:44
@jsumners
Copy link
Copy Markdown
Member

jsumners commented Dec 9, 2021

I don't see any issue with this.

@dalisoft
Copy link
Copy Markdown
Member

dalisoft commented Dec 13, 2021

@Eomm Any performance comparison would be helpful
At least here for users could see how performance differs than fast-json-stringify

Copy link
Copy Markdown
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.

I would call the option jtdSerializer:

@Eomm
Copy link
Copy Markdown
Member Author

Eomm commented Oct 23, 2022

Must add small benchmarks

@Eomm Eomm marked this pull request as ready for review October 25, 2022 06:36
Copy link
Copy Markdown
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

Copy link
Copy Markdown
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.

Looks good to me.

@Eomm Eomm merged commit be48a4c into main Oct 31, 2022
@Eomm Eomm deleted the ajv-serializer branch October 31, 2022 08:19
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.

ajv JSON serialization

4 participants