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

Update for hapi 17.x.x #61

Merged
merged 5 commits into from Nov 26, 2017
Merged

Update for hapi 17.x.x #61

merged 5 commits into from Nov 26, 2017

Conversation

@mtharrison
Copy link
Member

mtharrison commented Nov 11, 2017

Would appreciate reviews from people more familiar with async/await etc to give feedback on the approach taken.

Main changes:

  • Use of async/await
  • Rename of validateFunc option to validate
  • instead of returning err, isValid, credentials to validateFunc callback it now returns an object or throws an error
  • Previous ability to return a non-err error in err position (e/g/ to support redirects) has been supported here by instead returning a takeoverresponse property of the return object. I did consider just having it return a response object but not sure there's a way to check if it is a response object in the scheme?

Closes #60

Copy link
Member

Marsup left a comment

That's all I could find, didn't review the tests that deeply.

README.md Outdated
const main = async () => {
const server = new Hapi.Server({ port: 4000 });

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 11, 2017

Member

No new.

This comment has been minimized.

Copy link
@mtharrison

mtharrison Nov 12, 2017

Author Member

👍

@@ -31,22 +33,47 @@ const users = {
}
};
const validate = function (request, username, password, callback) {
const validate = async (request, username, password, h) => {

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 11, 2017

Member

What's the point of h if you're not using it ? Should there be an example with it being used ?

This comment has been minimized.

Copy link
@mtharrison

mtharrison Nov 12, 2017

Author Member

I'll remove it from here as it's not used. Don't need to show all functionality in the example.

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 12, 2017

Member

Is there any case where you would use it ?

This comment has been minimized.

Copy link
@mtharrison

mtharrison Nov 12, 2017

Author Member

Yes, you can set the return object response property to a custom response. Previously this was passed in the error position but it doesn't seem appropriate for it to be thrown.

You would use h to do something like:

return { response: h.redirect('http://hapijs.com') };

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 12, 2017

Member

May be worth of an example then ?

This comment has been minimized.

Copy link
@mtharrison

mtharrison Nov 13, 2017

Author Member

Sure, added to the README: d6396b9

"boom": "3.x.x",
"hoek": "4.x.x"
"boom": "7.x.x",
"bounce": "^1.2.0",

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 11, 2017

Member

Bounce not used ?

This comment has been minimized.

Copy link
@mtharrison

mtharrison Nov 12, 2017

Author Member

👍

if (!authorization) {
return reply(Boom.unauthorized(null, 'Basic', settings.unauthorizedAttributes));
return h.unauthenticated(Boom.unauthorized(null, 'Basic', settings.unauthorizedAttributes));

This comment has been minimized.

Copy link
@hueniverse

hueniverse Nov 12, 2017

Member

If you are not also passing credentials, you can just throw the error.

This comment has been minimized.

Copy link
@mtharrison

mtharrison Nov 12, 2017

Author Member

👍


credentials = credentials || null;
try {

This comment has been minimized.

Copy link
@hueniverse

hueniverse Nov 12, 2017

Member

This try scope is far too big. Declare the try variables with var and limit it to the one line.

This comment has been minimized.

Copy link
@mtharrison

mtharrison Nov 12, 2017

Author Member

Correct me if I'm wrong but I don't think we need the try/catch at all then, if it's ok to just have the error rethrown in this scope.


credentials = credentials || null;
try {
const { isValid, credentials, takeover } = await settings.validate(request, username, password, h);

This comment has been minimized.

Copy link
@hueniverse

hueniverse Nov 12, 2017

Member

Maybe response is a better variable name than takeover given the overlap with a hapi api.

This comment has been minimized.

Copy link
@mtharrison

mtharrison Nov 12, 2017

Author Member

Changed

mtharrison added 3 commits Nov 12, 2017
@Marsup
Marsup approved these changes Nov 13, 2017
@mtharrison mtharrison merged commit 222c98c into master Nov 26, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@mtharrison mtharrison deleted the v17 branch Nov 26, 2017
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 26, 2017

Please set milestones...

@mtharrison mtharrison added this to the 5. milestone Nov 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.