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.

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

fix: standalone ajv schema #461

Merged
merged 10 commits into from Jun 13, 2022
Merged

fix: standalone ajv schema #461

merged 10 commits into from Jun 13, 2022

Conversation

climba03003
Copy link
Member

@climba03003 climba03003 commented Jun 10, 2022

This PR do not need to merge immediately.
We need to test each edge case before merging this PR.

  1. It remove fast-json-stringify in standalone code.
  2. It address the issue of fast-json-stringify generate custom json schema.
  3. It update the types.

Checklist

@climba03003
Copy link
Member Author

Given that people complain fastify/fastify#3998 it about the fast-json-stringify/serialize apporach.
I see the point of @ivan-tymoshenko is absolutely right.

This patch will remove the dependent of fast-json-stringify internal files.

README.md Outdated Show resolved Hide resolved
Co-authored-by: Frazer Smith <frazer.dev@outlook.com>
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.

Good work, one nit

index.d.ts Outdated Show resolved Hide resolved
Co-authored-by: Matteo Collina <hello@matteocollina.com>
standalone.js Outdated Show resolved Hide resolved
@mcollina
Copy link
Member

lmk when it's ok to release

@ivan-tymoshenko
Copy link
Member

@climba03003 Do we need to generate a standalone serializer only here?
https://github.com/fastify/fastify/blob/main/build/build-error-serializer.js

@mcollina
Copy link
Member

I would not exclude we could need to do it elsewhere too.

Copy link
Member Author

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

@mcollina
I think it is ok to land now.
One final tweak is that it should not write ajv to standalone script when it is not needed.

index.d.ts Outdated Show resolved Hide resolved
Co-authored-by: Zed <zedeus@pm.me>
index.d.ts Outdated Show resolved Hide resolved
Co-authored-by: Zed <zedeus@pm.me>
@mcollina mcollina merged commit 12fa1e9 into master Jun 13, 2022
@mcollina mcollina deleted the fix-standalone-ajv-schema branch June 13, 2022 10:08
@mcollina
Copy link
Member

@climba03003 could you send the PRs needed to fastify and https://github.com/fastify/fast-json-stringify-compiler? Please update the versions everywhere.

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.

None yet

6 participants