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

Support hapi 17 #26

Merged
merged 23 commits into from
Apr 30, 2018
Merged

Support hapi 17 #26

merged 23 commits into from
Apr 30, 2018

Conversation

anthony-dibenedetto-olx
Copy link
Contributor

What does this PR do?

  • Adds support for Hapi 17

Related to issue #24

@anthony-dibenedetto-olx
Copy link
Contributor Author

@chapel @hueniverse

Should I update also the .travis.yml file?

@hueniverse
Copy link
Contributor

Yep, to match the other modules.

@anthony-dibenedetto-olx
Copy link
Contributor Author

anthony-dibenedetto-olx commented Mar 22, 2018

@hueniverse

Updated travis file. Also made some refactors and fixed some tests to reach 100% coverage

@anthony-dibenedetto-olx
Copy link
Contributor Author

@hueniverse @chapel

Could you take a look at this PR? Thanks!

@hueniverse
Copy link
Contributor

Need to find someone who uses this to review.

@jamesgibson14
Copy link

I have tested this out with my app and the caching is working, one thing I noticed though is that if the memcached server goes down after startup this.isConnected is still true so the request just gets stuck with no timeout/error

@hueniverse
Copy link
Contributor

@anthony-dibenedetto-olx Can you look into this issue?

@iniva
Copy link
Contributor

iniva commented Apr 28, 2018

@hueniverse
Yes I can take a look.

Meanwhile @jamesgibson14 if you want to help I wouldn't mind.

Eran, how do I / we proceed on this case? This PR is still unmerged and if James wanted to help he wouldn't be able to send a PR, unless it is against this branch.

Another thing I noticed is that this module doesn't have releases, unlike the other catbox-* modules. Is there a reason for that?

@hueniverse
Copy link
Contributor

I was unable to get any response from @chapel who is the only one with access rights on npm. I've emailed him and waiting to hear back. You should be able to merge the code to the repo though. Let me know if you were not added (via your personal account as requested).

@iniva
Copy link
Contributor

iniva commented Apr 29, 2018

@hueniverse yes, I can merge the code. I'll do that

@iniva iniva merged commit c725b90 into hapijs:master Apr 30, 2018
This was referenced Apr 30, 2018
@geek geek added the breaking changes Change that can breaking existing code label Apr 30, 2018
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking changes Change that can breaking existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants