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

feat: upgrade to @hapi #201

Merged
merged 11 commits into from
Sep 21, 2019
Merged

feat: upgrade to @hapi #201

merged 11 commits into from
Sep 21, 2019

Conversation

mkg20001
Copy link
Contributor

This upgrades all packages and switches namespaces for old hapi packages to new namespace, to fix warnings and security issues.

@mkg20001
Copy link
Contributor Author

Joi seems to have ditched the Joi.validate error which makes some tests fail

Also sinon is complaining about double-wrapped methods

Among other random test failures

@JKHeadley
Copy link
Owner

@mkg20001 This is great! Thanks for handling this. Definitely good to have loggin updated until #150 is handled.

Once we can get these tests fixed I'll go ahead and merge.

@mkg20001
Copy link
Contributor Author

@JKHeadley Some deps are still downgraded (since I tried fixing the tests this way, but seems that most are broken because of hapi API changes). Also joi-objectid has a problem with the new joi, so that is forked now as well.

Btw, I have sent an email to the owner of the loggin package and CC'd npm support, so if it won't get updated I'll possibly get to take it over next month.

I'll do the same one for joi-objectid, since peeble is now bought by fitbit and I doubt they're doing much opensource for it now.

@JKHeadley
Copy link
Owner

@mkg20001 Awesome about loggin and joi-objectId, def let me know how it goes!

@mkg20001
Copy link
Contributor Author

mkg20001 commented Sep 21, 2019

@JKHeadley I now own loggin (and the underlying gh-organization fistlabs)

In case of joi-objectid: I'll let the pr idle until the end of the month before contacting support.

(Edit: I've saved the email as a draft, could you possibly remind me in a few weeks to send it?)

@mkg20001
Copy link
Contributor Author

@JKHeadley Besides that, could you look into the tests? I'm clueless as to what is broken, but since you've written it you might be able to get a better idea.

@JKHeadley
Copy link
Owner

Great news! Yeah I'll try to look into the tests.

@mkg20001
Copy link
Contributor Author

mkg20001 commented Sep 21, 2019

Note that a possible bugfix for joi errors that include something about $._.alternatives is to wrap the Joi object {key: Joi.string()} with the joi function Joi.object({key: Joi.string()})

@codecov
Copy link

codecov bot commented Sep 21, 2019

Codecov Report

Merging #201 into master will not change coverage.
The diff coverage is 97.21%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #201   +/-   ##
======================================
  Coverage    94.3%   94.3%           
======================================
  Files          51      51           
  Lines       11827   11827           
======================================
  Hits        11154   11154           
  Misses        673     673
Impacted Files Coverage Δ
tests/unit/handler-helper.tests.js 98.15% <ø> (ø) ⬆️
tests/unit/rest-helper-factory.tests.js 97.59% <ø> (ø) ⬆️
tests/e2e/basic-non-embed.tests.js 99.7% <100%> (ø) ⬆️
tests/unit/query-helper.tests.js 99.18% <100%> (ø) ⬆️
policies/track-duplicated-fields.js 92.5% <100%> (ø) ⬆️
tests/e2e/audit-log.tests.js 100% <100%> (ø) ⬆️
...o-3/models/linking-models/user_permission.model.js 100% <100%> (ø) ⬆️
policies/enforce-document-scope.js 92.25% <100%> (ø) ⬆️
tests/e2e/duplicate-field.tests.js 100% <100%> (ø) ⬆️
...e2e/test-scenarios/scenario-1/models/role.model.js 100% <100%> (ø) ⬆️
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef5d919...bf9e261. Read the comment docs.

@mkg20001
Copy link
Contributor Author

I've sent out the email to joi-objectid now instead of waiting forever. Response is that it's a bit complicated since the company that owns it is now gone.

@JKHeadley
Copy link
Owner

Pushed some changes to fix the tests. I'll leave it up to you, but I think for now keeping Joi to v14 will be a good idea so we can merge the rest. The jump from 14 to 16 looks to be pretty major:
hapijs/joi#2037

Maybe we can tackle that one in a different PR.

@mkg20001
Copy link
Contributor Author

mkg20001 commented Sep 21, 2019

Sure, sounds pretty reasonable
Since they're both backwards compatible this shouldn't be much of an issue
Edit: That is, I'd also support doing this in a different pr

@mkg20001
Copy link
Contributor Author

So LGTM?

@JKHeadley JKHeadley merged commit a11cbfc into JKHeadley:master Sep 21, 2019
@JKHeadley
Copy link
Owner

Merged! Thanks for all the work! 👍

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.

2 participants