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 support for latest Joi #1178

Merged
merged 1 commit into from
Sep 21, 2018
Merged

Fix support for latest Joi #1178

merged 1 commit into from
Sep 21, 2018

Conversation

mcollina
Copy link
Member

I have not added any unit test because we cannot really test latest joi as it requires Node 8+. It works on my machine.

cc @cemremengu

Fixes #1158.

Checklist

  • run npm run test and npm run benchmark
  • tests and/or benchmarks are included
  • commit message and code follows Code of conduct

Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

Is this the only way to support it?
I would prefer a general purpose tool, such as the schema compiler instead have a custom specific code just to support an external library.

@mcollina
Copy link
Member Author

I can check if it is not a literal (e.g it has a constructor headers.constructor !== Object) but I think this is cleaner and we can extend that if there si a need. The reality is that we shouldn't be implying json schema in that specific function.

@delvedor
Copy link
Member

I think it would be better to figure out a general purpose approach.
Add this kind of code binds us to a specific library and change something in the future could lead to massive breaking changes. As far as I remember this is not the first time that Joi causes some issue...

@mcollina
Copy link
Member Author

@delvedor PTAL this is generic.

@cemremengu
Copy link
Contributor

cemremengu commented Sep 21, 2018

I guess a more general purpose approach than this would be extracting validation into a plugin and just provide the interface inside the core? Then we could have:

fastify-validation-ajv
fastify-validation-joi

Just some random thoughts

Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

LGTM

@cemremengu cemremengu merged commit 0afa858 into master Sep 21, 2018
@mcollina mcollina deleted the fix-support-for-latest-joi branch September 21, 2018 13:55
@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 Feb 14, 2022
@Eomm Eomm added the semver-minor Issue or PR that should land as semver minor label Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
semver-minor Issue or PR that should land as semver minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants