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

Joi 16 Compatibility #617

Merged
merged 15 commits into from
Nov 11, 2019
Merged

Joi 16 Compatibility #617

merged 15 commits into from
Nov 11, 2019

Conversation

Tornquist
Copy link
Contributor

@Tornquist Tornquist commented Oct 2, 2019

Summary

  • Upgrade all @hapi dependencies to current version. Major change from Joi 15 to 16.
  • Wrap hapi validation blocks to force hapi to use joi 16 instead of 15 (the packaged default)
  • Update lib/utilities, lib/paths, lib/properties to use new internal joi object structure
  • Update all tests

@Tornquist Tornquist mentioned this pull request Oct 2, 2019
@robmcguinness
Copy link
Collaborator

@Tornquist the unit test seem to pass. Is this ready for review?

@robmcguinness
Copy link
Collaborator

robmcguinness commented Oct 24, 2019

actually nm :) 13 of 198 tests failed. i'll try and take a peek.

@matejdr
Copy link
Contributor

matejdr commented Nov 7, 2019

Hi everyone, love the fact you are upgrading this. I kinda need this for one of my projects as well.
Had a quick look and I fixed 4 failed tests, mind adding me as a writer?

or just change these lines:
lib/responses.js:142
description: Hoek.reach(joiObj, '_description'),
to
description: Hoek.reach(joiObj, '_flags.description'),

lib/responses.js:97
if (Hoek.reach(userDefindedSchemas[key], 'schema.isJoi') && userDefindedSchemas[key].schema.isJoi === true) {
to
if (Hoek.reach(userDefindedSchemas[key], 'schema') && Utilities.isJoi(userDefindedSchemas[key].schema) === true) {

I can have a look at the remaining failed tests if you want.

@Tornquist
Copy link
Contributor Author

@matejdr Just make a PR into my branch.

@robmcguinness
Copy link
Collaborator

awesome let's keep this going

- forced a rule in one case as schema not supported in swagger
- fixed one test for alternatives as the actual models are different
@matejdr
Copy link
Contributor

matejdr commented Nov 8, 2019

@Tornquist , can you give me write access to your repo: git@github.com:Tornquist/hapi-swagger.git
At least to create new branches and create pull requests.

I don't want to fork the repo for the purpose of a single commit.

I fixed all the issues by the way, all tests pass.
Fixes for 2 tests are questionable though and I'll let you decide.

@Tornquist
Copy link
Contributor Author

Tornquist commented Nov 10, 2019

@matejdr I had missed that you put a PR up, for some reason I didn't get the notification.

Thanks for resolving these! I'll review shortly.

@coveralls
Copy link

coveralls commented Nov 10, 2019

Coverage Status

Coverage decreased (-0.6%) to 98.823% when pulling 3921811 on Tornquist:joi-16 into 9bb39ea on glennjones:master.

@Tornquist
Copy link
Contributor Author

Tornquist commented Nov 10, 2019

All tests are working. The coverage dropping below 99% is causing travis to fail.

Most of the lines reported as missed are required for the test suite to run successfully. A few are fallback cases for non-joi objects being sent in for validation. These blocks will be required again when hapi is not shipping a different joi version internally, but for now are not called.

@robmcguinness
Copy link
Collaborator

@Tornquist ready for review?

@robmcguinness robmcguinness marked this pull request as ready for review November 10, 2019 22:16
Copy link
Collaborator

@robmcguinness robmcguinness left a comment

Choose a reason for hiding this comment

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

LGTM. There is a coverage drop but I don't think it is worth holding up the release

README.md Outdated Show resolved Hide resolved
@Tornquist
Copy link
Contributor Author

@robmcguinness I believe it's about ready. I dropped the lab coverage threshold to 98 locally so that I could verify the full run with the Typescript checking afterward. There are hapi changes in Boom that are not contained the in @types package, and it looks like typescript type checking is currently broken if you're using the most recent packages.

To wrap this up for a final happy run, what are your thoughts on:

  1. Dropping the coverage min to 98%
  2. Removing the typescript checking

I am generally uncomfortable with removing/lowering checks, but I'm also not a fan of leaving them in as-if this was passing those marks.

Ref: hapijs/boom#251

Copy link
Contributor Author

@Tornquist Tornquist left a comment

Choose a reason for hiding this comment

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

I just gave it all another once-over and I'm happy with where it is.

@robmcguinness robmcguinness merged commit 74fb259 into hapi-swagger:master Nov 11, 2019
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

5 participants