Update to hapi v17. #104

WesTyler · 2017-10-21T16:27:56Z

Updates to hapi v17 and lab v15

Breaking Changes:

Remove "connections" and "onPreConnection" from manifest.
Return async function instead of using a callback or native Promise.
Change "preRegister" contract from callback to async function.
Change manifest shape from manifest.registrations array to manifest.register.plugins array.
Change expected plugin object shape to match hapi v17 expectations.

Non-Breaking Changes:

Hand plugin and registration options straight to hapi for validation at registration time.
Single server.register call instead of one per plugin.

csrl

What do you mean by 'move plugin attributes from children of "register" to siblings of "register"'? I don't see a need for any structure change to the manifest.

csrl · 2017-10-21T19:08:14Z

lib/index.js

        let path = plugin.register;
        if (relativeTo && path[0] === '.') {
            path = Path.join(relativeTo, path);
        }

-        plugin.register = require(path);
+        pluginObject.plugin = require(path);


I don't understand this change - the required field is 'register' not 'plugin'. I don't see any change regarding that in the hapi 17 docs.

It's entirely possible that I'm misunderstanding, but here is the change I'm looking at.

Switch plugins to use a simple object with a register property. All the other attributes move to the object alongside register.

and

Change server.register() to support { plugin, options } instead of { register, options }

Additionally (and this may very well be an error on my side) when I try to register the plugins using

exports.register = function (server, options) { server.expose('hello', options.who || 'world'); }; exports.register.attributes = { name: 'helloworld', multiple: false };

as it was before, I get the error:

Error: Invalid plugin options { "plugin": { "register": function (server, options) {\n\n server.expose('hello', options.who || 'world');\n}, "name" [1]: -- missing -- } } [1] "name" is required

I misread the documentation.

I'm tempted to have glue only call server.register() a single time, passing all plugins at once.

In which case, the new schema would be:

{ options: Joi.object({ relativeTo: Joi.string(), preRegister: Joi.func().allow(false) }), manifest: Joi.object({ server: Joi.object(), register: Joi.Object({ plugins: Joi.array().items( Joi.string(), Joi.object() ), options: Joi.any() )) }) }

example manifests:

{register: { plugins: ["myplugin"] }}

{register: { plugins: [require("myplugin")], options: registerOptions }}

{register: { plugins: [{plugin: "myplugin", options: myPluginOptions}] }}

{register: { plugins: [ {plugin: "myplugin", options: myPluginOptions}, {plugin: "otherplugin", options: otherPluginOptions, routes: overrideRegisterRoutesOption}, "thirdplugin", {plugin: require("bestplugin"), options: bestsPluginOptions} ], options: registerOptions }}

And from that, if an item from the register.plugins array is a string we'll require() it, otherwise we'll pass the register.plugins array directly to server.register() without any additional validation, and let hapi itself validate the input. As far as that goes, I'd be fine with simplifying the manifest schema to be:

Joi.object({ server: Joi.object(), register: Joi.Object({ plugins: Joi.array(), options: Joi.any() )) })

As we don't care what format the registration options are, and we don't care what the plugin format is, except if its a string, we require() it. Let them pass whatever to hapi, where hapi is the source of truth to validate what is provided.

I like that. I'll do that refactor on this PR this weekend 👍

csrl · 2017-10-21T19:08:56Z

lib/index.js

-    if (typeof plugin === 'string') {
-        plugin = { register: plugin };
+    const pluginObject = { plugin };
+    if (typeof plugin.options === 'object') {


'plugin' may be a string here, so its premature to reference 'plugin.options'

Yep, the function still needs some love based on other changes still in the air 👍

WesTyler · 2017-10-23T21:54:23Z

Okay, I took a first pass at the changes we discussed above. I also tried to update the API.md to match. I don't love the wording in some parts, but I could use some input on how to make it more clear.

Is this implementation what you had in mind?

csrl · 2017-10-25T06:15:55Z

I pushed a work in progress review commit to 'review/pr104' branch to give you an idea of how I'm looking at it. If that makes sense to you, and you want to finish it off (updating the rest of the tests etc), please do.

WesTyler · 2017-10-25T14:38:57Z

Ohhh excellent yes. I'm actually very happy to be removing Items and going to async/await. 👍 👍 👍

I'll wrap those changes in here this afternoon. Thanks!

WesTyler · 2017-10-25T22:01:28Z

Alright, I did the async/await refactor and also went ahead and updated dev dependencies to lab@15 and code@5 to make use of the async/await features there.

csrl · 2017-10-28T22:58:54Z

lib/index.js

-            ],
-            options: Joi.object()
-        }))
+        server  : Joi.object(),


no spaces before the colon

csrl · 2017-10-28T22:59:54Z

lib/index.js

-            });
-        });
-    }
-
    // Create server


might as well drop this comment - there are no other comments, and its not really pertinent to call out.

csrl · 2017-10-28T23:02:30Z

README.md

@@ -8,20 +8,20 @@ Lead Maintainer - [Chris Rempel](https://github.com/csrl)

 Glue provides configuration based composition of hapi's Server object. Specifically it wraps

- * `server = new Hapi.Server(Options)`
- * one or more `server.connection(Options)`
+ * `server = Hapi.server(Options)`
 * zero or more `server.register(Plugin, Options)`


it now wraps a single server.register call

csrl · 2017-10-28T23:03:14Z

README.md

 * zero or more `server.register(Plugin, Options)`

 calling each based on the configuration generated from the Glue manifest.

 ### Interface

-Glue's [API](API.md) is a single function `compose` accepting a JSON `manifest` file specifying the hapi server options, connections, and registrations.
+Glue's [API](API.md) is a single function `compose` accepting a JSON `manifest` file specifying the hapi server options and registrations.


remove the word "file". It doesn't interact with any files.

csrl

I just release 4.2.1 to fix the promise bug. So if you can rebase your PR ontop of current master, then squash the commits into a single one with a final commit message, I'll do a final review and merge it in. Thanks.

WesTyler · 2017-10-29T15:17:17Z

Done and done! Thanks for the help getting this where it needed to be @csrl

csrl · 2017-10-30T23:06:06Z

I think I will hold off on releasing this until hapi 17 final lands. We could do a release candidate, but I'm not too motivated to do so. So if you want to update the package.json to reference hapi 17 instead of rc9, then it'll be a simple merge whenever hapi 17 is released. thoughts?

WesTyler · 2017-10-31T02:50:32Z

Yeah, that sounds good to me 👍

I'll keep an eye on the state of the branch if Eran does anymore rcs just to make sure.

Breaking Changes: - Remove "connections" and "onPreConnection" from manifest. - Return async function instead of using a callback or native Promise. - Change "preRegister" contract from callback to async function. - Change manifest shape from manifest.registrations array to manifest.register.plugins array. - Change expected plugin object shape to match hapi v17 expectations. Non-Breaking Changes: - Hand plugin and registration options straight to hapi for validation at registration time. - Single server.register call instead of one per plugin.

WesTyler · 2017-10-31T14:42:10Z

Also, the Travis build is going to fail since there is no install target yet for "hapi": "17.x.x". Just throwing that out there before a big red X appears next to my commit.

WesTyler · 2017-11-04T15:21:55Z

Looks like we are all good with the release of 17.0.0 yesterday.

csrl · 2017-11-06T00:13:44Z

Thanks @WesTyler. Nice work on this release. I'm actually not sure when I'll be updating to hapi 17 for the main project I work on with hapi. Do you have any interesting in taking over as maintainer for glue?

WesTyler · 2017-11-06T15:09:55Z

Nice work on this release

Thank you, and thank you for the help with it!

Do you have any interesting in taking over as maintainer for glue?

If you are wanting to pass it off, then absolutely! We will be updating to hapi 17 and glue 5 at work soon, so I should have plenty of time to be hands on with it.

csrl · 2017-11-07T00:13:54Z

ok, I'll let you know - we'll make a decision next week on when/if we will port to hapi 17.

WesTyler · 2017-11-07T00:23:41Z

Sounds good 👍

…

On Mon, Nov 6, 2017 at 6:13 PM csrl ***@***.***> wrote: ok, I'll let you know - we'll make a decision next week on when/if we will port to hapi 17. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#104 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALl4O_6Gap_jf_3zAm5fUWJoXxrrFflzks5sz6DDgaJpZM4QBn08> .

lock · 2020-01-09T01:06:45Z

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.

WesTyler mentioned this pull request Oct 21, 2017

Hapi 17 #102

Closed

csrl reviewed Oct 21, 2017

View reviewed changes

csrl added the breaking changes Change that can breaking existing code label Oct 21, 2017

csrl reviewed Oct 28, 2017

View reviewed changes

lib/index.js Outdated

],

options: Joi.object()

}))

server : Joi.object(),

Copy link

Contributor

csrl Oct 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no spaces before the colon

csrl reviewed Oct 28, 2017

View reviewed changes

csrl suggested changes Oct 28, 2017

View reviewed changes

csrl approved these changes Oct 31, 2017

View reviewed changes

csrl merged commit 465939b into hapijs:master Nov 6, 2017

csrl added this to the 5.0.0 milestone Nov 6, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to hapi v17. #104

Update to hapi v17. #104

WesTyler commented Oct 21, 2017 •

edited

Loading

csrl left a comment

csrl Oct 21, 2017

WesTyler Oct 21, 2017 •

edited

Loading

csrl Oct 21, 2017

WesTyler Oct 21, 2017

csrl Oct 21, 2017

WesTyler Oct 21, 2017

WesTyler commented Oct 23, 2017

csrl commented Oct 25, 2017

WesTyler commented Oct 25, 2017

WesTyler commented Oct 25, 2017

csrl Oct 28, 2017

csrl Oct 28, 2017

csrl Oct 28, 2017

csrl Oct 28, 2017

csrl left a comment

WesTyler commented Oct 29, 2017

csrl commented Oct 30, 2017

WesTyler commented Oct 31, 2017

WesTyler commented Oct 31, 2017 •

edited

Loading

WesTyler commented Nov 4, 2017

csrl commented Nov 6, 2017

WesTyler commented Nov 6, 2017

csrl commented Nov 7, 2017

WesTyler commented Nov 7, 2017 via email

lock bot commented Jan 9, 2020

Update to hapi v17. #104

Update to hapi v17. #104

Conversation

WesTyler commented Oct 21, 2017 • edited Loading

csrl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesTyler Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesTyler commented Oct 23, 2017

csrl commented Oct 25, 2017

WesTyler commented Oct 25, 2017

WesTyler commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csrl left a comment

Choose a reason for hiding this comment

WesTyler commented Oct 29, 2017

csrl commented Oct 30, 2017

WesTyler commented Oct 31, 2017

WesTyler commented Oct 31, 2017 • edited Loading

WesTyler commented Nov 4, 2017

csrl commented Nov 6, 2017

WesTyler commented Nov 6, 2017

csrl commented Nov 7, 2017

WesTyler commented Nov 7, 2017 via email

lock bot commented Jan 9, 2020

WesTyler commented Oct 21, 2017 •

edited

Loading

WesTyler Oct 21, 2017 •

edited

Loading

WesTyler commented Oct 31, 2017 •

edited

Loading