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

add unit test for ajv-formats #4187

Merged
merged 1 commit into from Aug 10, 2022

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Aug 10, 2022

We have ajv-formats in devDependencies but dont use it anywhere. In order to check if ajv-formats does work in regards to fastify/help#732 I wrote the test to ensure that it should work.

Checklist

@github-actions github-actions bot added the test Issue or pr related to our testing infrastructure. label Aug 10, 2022
const fastify = Fastify({
ajv: {
plugins: [
require('ajv-formats')
Copy link
Member

Choose a reason for hiding this comment

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

It is actually added by @fastify/ajv-compiler.

The only exception is when you apply your own ajv-formats, it will override the ajv-formats in @fastify/ajv-compiler

Copy link
Contributor

Choose a reason for hiding this comment

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

I had no idea ajv-formats was included with Fastify via @fastify/ajv-compiler — that's great! I don't see it mentioned in the Validation documentation, but I'd be happy to open a PR that adds this in.

As far as I can tell, ajv-formats wasn't included with Fastify v3 — is that correct?

Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell, ajv-formats wasn't included with Fastify v3 — is that correct?

No, ajv-formats is embedded inside ajv@6.
It is extracted in ajv@7 and we need to explicitly add it through ajv-formats.

See More: https://github.com/ajv-validator/ajv/releases/tag/v7.0.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh that's right, I forgot about that. And Fastify went from Ajv v6 (formats embedded) -> v8 (formats in ajv-formats)?

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
Copy link
Member

I guess an added unit test is always useful!

@Uzlopak Uzlopak merged commit ea8aae9 into fastify:main Aug 10, 2022
@Uzlopak Uzlopak deleted the add-unit-test-for-ajv-formats branch August 10, 2022 14:39
philippviereck added a commit to philippviereck/fastify that referenced this pull request Aug 11, 2022
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
test Issue or pr related to our testing infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants