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

Upgraded module to hapi v17 #174

Merged
merged 11 commits into from Dec 9, 2017
Merged

Upgraded module to hapi v17 #174

merged 11 commits into from Dec 9, 2017

Conversation

@mrlannigan
Copy link
Contributor

mrlannigan commented Nov 7, 2017

Breaking change

  • Updates validateFunc function signature, see Readme.

This includes updates to all tests, example, and documentation.

@mrlannigan mrlannigan mentioned this pull request Nov 7, 2017

expect(err).to.not.exist();
server.auth.strategy('session', 'cookie', { password: new Buffer('foobar') });

This comment has been minimized.

Copy link
@vinicius0026

vinicius0026 Nov 7, 2017

new Buffer is deprecated since node v6. Should be replaced by Buffer.from.

This comment has been minimized.

Copy link
@mrlannigan

mrlannigan Nov 7, 2017

Author Contributor

Good catch! Updated.

@mrlannigan mrlannigan mentioned this pull request Nov 7, 2017

const server = new Hapi.Server();
server.connection();
server.register(require('../'), (err) => {
server.register(require('../'));

This comment has been minimized.

Copy link
@vinicius0026

vinicius0026 Nov 7, 2017

Shouldn't you await on server.register as per the new docs?

This comment has been minimized.

Copy link
@nlf

nlf Nov 7, 2017

Member

yes, you should


server.auth.strategy('default', 'cookie', true, { validateFunc: 'not a function' });
}).to.throw(Error);
/* eslint-disable hapi/no-shadow-relaxed */

This comment has been minimized.

Copy link
@vinicius0026

vinicius0026 Nov 7, 2017

I guess this flag is no longer necessary

This comment has been minimized.

Copy link
@nlf

nlf Nov 7, 2017

Member

yeah, if you need to turn off linting in your tests your tests are broken - it looks like you've fixed the error this was meant to circumvent anyway though

Copy link
Member

nlf left a comment

i added some comments, but overall this is looking really great.

the "fat arrow" related comments, i'm more or less neutral as to whether you use fat arrow functions or not, but they should at least be consistent.

README.md Outdated
};
const logout = function (request, reply) {
const logout = function (request, h) {

This comment has been minimized.

Copy link
@nlf

nlf Nov 7, 2017

Member

fat arrow functions

README.md Outdated
};
const server = new Hapi.Server();
server.connection({ port: 8000 });
const server = new Hapi.Server({ port: 8000 });

This comment has been minimized.

Copy link
@nlf

nlf Nov 7, 2017

Member

const server = Hapi.server({ port: 8000 });

README.md Outdated
validateFunc: function (request, session, callback) {
cache.get(session.sid, (err, cached) => {
validateFunc: async function (request, session) {

This comment has been minimized.

Copy link
@nlf

nlf Nov 7, 2017

Member

fat arrow

README.md Outdated
exports.start();
}
catch (err) {
console.log(err.stack);

This comment has been minimized.

Copy link
@nlf

nlf Nov 7, 2017

Member

i feel like there should be a process.exit(1); here, as-is this would log an error but exit with a non-error exit code, that seems bad

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 8, 2017

Member

I doubt the catch would ever be triggered since node doesn't have top level async/await.

This comment has been minimized.

Copy link
@nlf

nlf Nov 8, 2017

Member

good point, it would be a promise rejection, so exports.start().catch((err) => {}); is more appropriate

This comment has been minimized.

Copy link
@mrlannigan

mrlannigan Nov 9, 2017

Author Contributor

So what do you all think about wrapping this in a self-executing async function?

This comment has been minimized.

Copy link
@mrlannigan

mrlannigan Nov 9, 2017

Author Contributor

Also, I didn't really want to make too large of a change in this PR aside from making it compatible with hapi17, but I'm not a huge fan of the fact the whole example/index.js being in the readme. I could simply keep the example updated and point to it from here.

@@ -12,19 +12,19 @@ const users = {
}
};

const home = function (request, reply) {
const home = function (request, h) {

This comment has been minimized.

Copy link
@nlf

nlf Nov 7, 2017

Member

fat arrow

};

const logout = function (request, reply) {
const logout = function (request, h) {

This comment has been minimized.

Copy link
@nlf

nlf Nov 7, 2017

Member

fat arrow

};

const server = new Hapi.Server();
server.connection({ port: 8000 });
const server = new Hapi.Server({ port: 8000 });

This comment has been minimized.

Copy link
@nlf

nlf Nov 7, 2017

Member

const server = Hapi.server({ port: 8000 });

This comment has been minimized.

Copy link
@wswoodruff

wswoodruff Nov 9, 2017

Are we not using new anymore? I thought I saw that somewhere.. maybe I was reading the wrong docs version!

This comment has been minimized.

Copy link
@mrlannigan

This comment has been minimized.

Copy link
@wswoodruff

wswoodruff Nov 9, 2017

Cool, thanks for the research!

password: 'password-should-be-32-characters',
cookie: 'sid-example',
redirectTo: '/login',
isSecure: false,
validateFunc: function (request, session, callback) {
validateFunc: async function (request, session) {

This comment has been minimized.

Copy link
@nlf

nlf Nov 7, 2017

Member

fat arrow



describe('scheme', () => {

it('fails with no plugin options', (done) => {
it('fails with no plugin options', () => {

const server = new Hapi.Server();

This comment has been minimized.

Copy link
@nlf

nlf Nov 7, 2017

Member

const server = Hapi.server()

exports.start();
}
catch (err) {
console.log(err.stack);

This comment has been minimized.

Copy link
@nlf

nlf Nov 7, 2017

Member

again, process.exit(1);

Copy link
Member

Marsup left a comment

I think now CookieAuth could be an actual class. Also don't forget to update the engines in package.json.

README.md Outdated
exports.start();
}
catch (err) {
console.log(err.stack);

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 8, 2017

Member

I doubt the catch would ever be triggered since node doesn't have top level async/await.


const CookieAuth = function () {
class CookieAuth {

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 9, 2017

Member

Probably the class can be extracted and request attached to it through the constructor.

This comment has been minimized.

Copy link
@mrlannigan

mrlannigan Nov 9, 2017

Author Contributor

Done, also had to attach the settings.

@toddhickerson

This comment has been minimized.

Copy link

toddhickerson commented Nov 23, 2017

Any ETA on when this will be completed, or should someone else provide a new PR?

@@ -12,20 +12,21 @@
"session"
],
"engines": {
"node": ">=4.x.x"
"node": ">=8.9.x"
},
"dependencies": {
"boom": "3.x.x",

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 23, 2017

Member

I think boom needs a well-deserved bump.

This comment has been minimized.

Copy link
@mrlannigan

mrlannigan Nov 24, 2017

Author Contributor

Good point, done.

@Marsup

This comment has been minimized.

Copy link
Member

Marsup commented Nov 23, 2017

@toddhickerson Why a new PR ?

@aknuds1

This comment has been minimized.

Copy link
Contributor

aknuds1 commented Nov 24, 2017

@nlf Are there any remaining issues now?

@toddhickerson

This comment has been minimized.

Copy link

toddhickerson commented Nov 24, 2017

@Marsup Not sure how the whole process works, but what is remaining to get this accepted? This is the last item holding me back from hapi v17 on two projects.

README.md Outdated
not trigger a redirection. Requires **hapi** version 6.2.0 or newer. Defaults to `true`;
- `validateFunc` - an optional session validation function used to validate the content of the
not trigger a redirection. Defaults to `true`;
- `await validateFunc` - an optional session validation function used to validate the content of the

This comment has been minimized.

Copy link
@hueniverse

hueniverse Nov 26, 2017

Member

This should be async here as it is the method provided by the user, not invoked by the user.

This comment has been minimized.

Copy link
@mrlannigan

mrlannigan Nov 27, 2017

Author Contributor

Makes sense, updated.

@aknuds1

This comment has been minimized.

Copy link
Contributor

aknuds1 commented Nov 26, 2017

I've tested this PR now, by upgrading to it in my app, and it works AFAICT.

not trigger a redirection. Requires **hapi** version 6.2.0 or newer. Defaults to `true`;
- `validateFunc` - an optional session validation function used to validate the content of the
not trigger a redirection. Defaults to `true`;
- `async validateFunc` - an optional session validation function used to validate the content of the

This comment has been minimized.

Copy link
@mtharrison

mtharrison Nov 30, 2017

Member

Do you think async should be here? It could confuse people, thinking that async is part of the property name? In hapi-auth-basic I put it as part of the signature: https://github.com/hapijs/hapi-auth-basic#hapi-auth-basic

This comment has been minimized.

Copy link
@hueniverse

hueniverse Nov 30, 2017

Member

In hapi docs its in the description signature.

README.md Outdated
console.log('Server ready');
};
exports.start().catch((err) => {

This comment has been minimized.

server.route({
method: 'GET', path: '/resource', handler: function (request, h) {

return h.response('resource');

This comment has been minimized.

Copy link
@mtharrison

mtharrison Nov 30, 2017

Member

Can just return it?

handler: function (request, h) {

request.cookieAuth.set({ user: request.params.user });
return h.response(request.params.user);

This comment has been minimized.

Copy link
@mtharrison

mtharrison Nov 30, 2017

Member

Can just return it?

This comment has been minimized.

Copy link
@mrlannigan

mrlannigan Dec 8, 2017

Author Contributor

You can.

if (settings.clearInvalid) {
reply.unstate(settings.cookie);
}
Hoek.assert(typeof result === 'object', 'Invalid return from validateFunc');

This comment has been minimized.

Copy link
@mtharrison

mtharrison Nov 30, 2017

Member

Here you're validating the user's own code. It feels a bit out of scope, they should have tests.

This comment has been minimized.

Copy link
@aknuds1

aknuds1 Dec 1, 2017

Contributor

Personally I find it useful that the framework verifies the results of user provided hooks for ease of debugging, unless the benefits are outweighed by performance concerns.

This comment has been minimized.

Copy link
@mtharrison

mtharrison Dec 1, 2017

Member

There's no harm in it I guess. I don't think there's any perf concerns with a couple of asserts.

@toddhickerson

This comment has been minimized.

Copy link

toddhickerson commented Dec 4, 2017

@mrlannigan I appreciate your work on this and am anxious to use it with Hapi v17. Are there any remaining issues to be addressed?

@aknuds1

This comment has been minimized.

Copy link
Contributor

aknuds1 commented Dec 4, 2017

I'm also anxious to see this being integrated, I'm already using it (via my namespaced package) :)

@BigWillie

This comment has been minimized.

Copy link

BigWillie commented Dec 8, 2017

Will this be accepted? Or should I go with the namespaced package: https://www.npmjs.com/package/@arve.knudsen/hapi-auth-cookie

@nlf

This comment has been minimized.

Copy link
Member

nlf commented Dec 8, 2017

@mrlannigan i'm going to make you the new maintainer of this module and let you merge and publish this at will. what's your npm username so we can get you permissions there?

@nlf

This comment has been minimized.

Copy link
Member

nlf commented Dec 8, 2017

also, you have a pending invitation to the hapijs org, iirc you can go to https://github.com/hapijs to accept it

@mrlannigan

This comment has been minimized.

Copy link
Contributor Author

mrlannigan commented Dec 8, 2017

@nlf Thank you!

@toddhickerson I am going to address @mtharrison changes then work on a release.

@nlf

This comment has been minimized.

Copy link
Member

nlf commented Dec 8, 2017

@mrlannigan what's your npm username?

@mrlannigan

This comment has been minimized.

Copy link
Contributor Author

mrlannigan commented Dec 9, 2017

Sorry @nlf, it's the same as my github username: mrlannigan

@nlf

This comment has been minimized.

Copy link
Member

nlf commented Dec 9, 2017

No worries, I just gave you publish permissions there. Module's all yours!

@mrlannigan mrlannigan added this to the 8.0.0 milestone Dec 9, 2017
@mrlannigan mrlannigan merged commit 49b2cd9 into hapijs:master Dec 9, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mrlannigan mrlannigan deleted the mrlannigan:feature/hapi17 branch Dec 9, 2017
@mtharrison

This comment has been minimized.

Copy link
Member

mtharrison commented Dec 11, 2017

It was merged without any approvals?

@nlf

This comment has been minimized.

Copy link
Member

nlf commented Dec 11, 2017

It’s his module now and his pull request, it’s on him to maintain as he sees fit

@mrlannigan

This comment has been minimized.

Copy link
Contributor Author

mrlannigan commented Dec 12, 2017

@mtharrison I felt like I had addressed all the blocking changes and next time I will definitely either explicitly ask for the disapproval to be reevaluated or I will dismiss it with an explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.