Upgraded module to hapi v17 by mrlannigan · Pull Request #174 · hapijs/cookie

mrlannigan · 2017-11-07T12:29:53Z

Breaking change

Updates validateFunc function signature, see Readme.

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

vinicius0026 · 2017-11-07T12:39:31Z

test/index.js

+        expect(() => {

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


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

Good catch! Updated.

vinicius0026 · 2017-11-07T13:31:14Z

test/index.js

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


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

yes, you should

vinicius0026 · 2017-11-07T13:33:03Z

test/index.js


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


I guess this flag is no longer necessary

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

nlf

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.

nlf · 2017-11-07T23:37:02Z

README.md

 };

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


fat arrow functions

nlf · 2017-11-07T23:37:25Z

README.md


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


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

nlf · 2017-11-07T23:37:45Z

README.md

-        validateFunc: function (request, session, callback) {
-
-            cache.get(session.sid, (err, cached) => {
+        validateFunc: async function (request, session) {


nlf · 2017-11-07T23:38:43Z

README.md

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


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

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

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

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

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.

nlf · 2017-11-07T23:38:54Z

example/index.js

 };

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


nlf · 2017-11-07T23:39:10Z

example/index.js

 };

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


nlf · 2017-11-07T23:39:24Z

example/index.js


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


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

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

https://hapijs.com/api/17.0.1#-serveroptions
compared to:
https://hapijs.com/api/16.6.2#new-serveroptions

And it looks like this is where it was changed to a factory method:
hapijs/hapi@f35b8c5#diff-c945a46d13b34fcaff544d966cffcabaR23

Cool, thanks for the research!

nlf · 2017-11-07T23:39:35Z

example/index.js

        redirectTo: '/login',
        isSecure: false,
-        validateFunc: function (request, session, callback) {
+        validateFunc: async function (request, session) {


nlf · 2017-11-07T23:43:29Z

test/index.js

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

        const server = new Hapi.Server();


const server = Hapi.server()

nlf · 2017-11-07T23:46:02Z

example/index.js

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


again, process.exit(1);

Marsup

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

Marsup · 2017-11-08T09:12:25Z

README.md

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


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

…in readme

Marsup · 2017-11-09T15:23:56Z

lib/index.js

+    const decoration = (request) => {

-        const CookieAuth = function () {
+        class CookieAuth {


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

Done, also had to attach the settings.

toddhickerson · 2017-11-23T16:31:37Z

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

Marsup · 2017-11-23T16:46:16Z

package.json

+    "node": ">=8.9.x"
  },
  "dependencies": {
    "boom": "3.x.x",


I think boom needs a well-deserved bump.

Good point, done.

Marsup · 2017-11-23T16:46:43Z

@toddhickerson Why a new PR ?

aknuds1 · 2017-11-24T17:55:32Z

@nlf Are there any remaining issues now?

toddhickerson · 2017-11-24T17:58:10Z

@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.

hueniverse · 2017-11-26T00:22:04Z

README.md

-  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 should be async here as it is the method provided by the user, not invoked by the user.

Makes sense, updated.

aknuds1 · 2017-11-26T11:22:53Z

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

mtharrison · 2017-11-30T05:14:39Z

README.md

-  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


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

In hapi docs its in the description signature.

mtharrison · 2017-11-30T05:16:51Z

README.md

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


hapijs/hapi#3684 (comment)

mtharrison · 2017-11-30T05:28:53Z

test/helpers.js

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


Can just return it?

mtharrison · 2017-11-30T05:28:55Z

test/helpers.js

+            handler: function (request, h) {
+
+                request.cookieAuth.set({ user: request.params.user });
+                return h.response(request.params.user);


Can just return it?

mtharrison · 2017-11-30T06:58:13Z

lib/index.js

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


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

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.

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

toddhickerson · 2017-12-04T02:08:55Z

@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 · 2017-12-04T08:00:12Z

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

BigWillie · 2017-12-08T20:29:17Z

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

nlf · 2017-12-08T22:25:15Z

@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 · 2017-12-08T22:43:57Z

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

mrlannigan · 2017-12-08T23:13:49Z

@nlf Thank you!

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

nlf · 2017-12-08T23:19:35Z

@mrlannigan what's your npm username?

mrlannigan · 2017-12-09T16:41:00Z

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

nlf · 2017-12-09T16:58:23Z

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

mtharrison · 2017-12-11T02:55:11Z

It was merged without any approvals?

nlf · 2017-12-11T05:28:09Z

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

mrlannigan · 2017-12-12T00:25:31Z

@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.

lock · 2020-01-09T01:50:12Z

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.

Upgraded module to hapi v17

2fccd79

mrlannigan mentioned this pull request Nov 7, 2017

Updating dependencies #173

Closed

vinicius0026 reviewed Nov 7, 2017

View reviewed changes

buffer.from

dbe3d65

mrlannigan mentioned this pull request Nov 7, 2017

Seeking new maintainer #153

Closed

vinicius0026 reviewed Nov 7, 2017

View reviewed changes

nlf requested changes Nov 7, 2017

View reviewed changes

Marsup reviewed Nov 8, 2017

View reviewed changes

mrlannigan added 5 commits November 8, 2017 22:03

removed constructor new from server create, awaited all server.registers

b481a19

fat arrow consistency

a20da59

updated catch handler for example's server start and updated example …

8ec708a

…in readme

updated engines

9309d6f

converted CookieAuth to a class

12de043

Marsup reviewed Nov 9, 2017

View reviewed changes

moved CookieAuth to the internals

0bb52e3

Marsup reviewed Nov 23, 2017

View reviewed changes

Bumped boom to v7

a9036b6

hueniverse reviewed Nov 26, 2017

View reviewed changes

Clarified validateFunc documentation

58a8bd1

mtharrison suggested changes Nov 30, 2017

View reviewed changes

mtharrison reviewed Nov 30, 2017

View reviewed changes

Async style improvement to example and simplified test handler routes

cfc46da

mrlannigan added this to the 8.0.0 milestone Dec 9, 2017

mrlannigan added the breaking changes Change that can breaking existing code label Dec 9, 2017

mrlannigan merged commit 49b2cd9 into hapijs:master Dec 9, 2017

mrlannigan deleted the feature/hapi17 branch December 9, 2017 17:49

lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020

Conversation

mrlannigan commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinicius0026 Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrlannigan Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marsup left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toddhickerson commented Nov 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marsup commented Nov 23, 2017

Uh oh!

aknuds1 commented Nov 24, 2017

Uh oh!

toddhickerson commented Nov 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrlannigan commented Nov 7, 2017 •

edited

Loading

vinicius0026 Nov 7, 2017 •

edited

Loading

mrlannigan Nov 9, 2017 •

edited

Loading

aknuds1 commented Nov 26, 2017 •

edited

Loading