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

Pass manager options at registration time #135

Merged
merged 16 commits into from Dec 29, 2017
Merged

Conversation

@wswoodruff
Copy link
Contributor

wswoodruff commented Nov 19, 2017

  • Feedback welcome

  • Set multiple: true, as it's required to pass plugin options

  • Register vision via server.register({ plugin: Vision, options: { ... }); where:

    • options is a manager config, is synonymous with server.views
  • Effectively, the only thing this adds is the ability to use

      await server.register({
          plugin: Vision,
          options: {/* view manager options */}
      });
    
      // which will have the same effect as
    
      await server.register(Vision);
      server.views({/* view manager options */});
    

const parent = realm.parent;

return internals.nearestState(parent);

This comment has been minimized.

Copy link
@devinivy

devinivy Nov 20, 2017

Member

Perhaps (?) not a practical concern here, but beware recursion in javascript. There's still no such thing as tail recursion in node, so each time you recurse the call stack gets longer (and it has a max allowed size).

This comment has been minimized.

Copy link
@devinivy

devinivy Nov 20, 2017

Member

(And this can always be expressed as a while loop!)

This comment has been minimized.

Copy link
@wswoodruff

wswoodruff Nov 20, 2017

Author Contributor

I was having trouble getting code coverage with the while loop Eran had here, because parent would never evaluate to false, since the root server always has vision state. The root server always has vision state because of the { setup: true } introduced in the register func.

Here's what's currently in vision - the loop Eran wrote:

internals.realm = function (server) {

    if (server.realm.plugins.vision) {
        return server.realm.plugins.vision;
    }

    let parent = server.realm.parent;
    while (parent) {
        if (parent.plugins.vision) {
            return parent.plugins.vision;
        }

        parent = parent.parent;
    }

    return {};
};

As soon as the root server goes in this loop, it is returned as having a realm because setup.true will always be there. So parent = parent.parent will never run, which would cause it to evaluate to false in the while loop, and return {}

This comment has been minimized.

Copy link
@wswoodruff

wswoodruff Nov 20, 2017

Author Contributor

This could possibly be rewritten as nearestManager, which would fix the issue of returning just because a realm has state in vision


internals.getRootRealm = (realm) => {

let rootRealm = realm;

This comment has been minimized.

Copy link
@devinivy

devinivy Nov 20, 2017

Member

I think rootRealm is a slightly odd variable name here– it's not the root realm until it's returned (and not being used as a variable anymore).

This comment has been minimized.

Copy link
@wswoodruff

wswoodruff Nov 20, 2017

Author Contributor

Agree!


Joi.assert(options, internals.schema.manager, 'Invalid manager options');

const realm = this.realm.parent || this.realm;

This comment has been minimized.

Copy link
@devinivy

devinivy Nov 20, 2017

Member

Wait, when server.views() is called inside a plugin wont this set the manager on the plugin's parent? It would be hard to see this in a test when nearestState() comes into play. But imagine server1 registers a plugin with server2, and server2 register a plugin with server3. Now server3 calls server3.views()– I think this will cause server2 and server3 to both have the same manager.

This comment has been minimized.

Copy link
@wswoodruff

wswoodruff Nov 20, 2017

Author Contributor

I think you're probably right, I'll definitely go back and ensure this acts correctly in tests

});
internals.assignManager = function (options) {

Joi.assert(options, internals.schema.manager, 'Invalid manager options');

This comment has been minimized.

Copy link
@devinivy

devinivy Nov 20, 2017

Member

Doesn't the manager make this assertion on its own?

This comment has been minimized.

Copy link
@wswoodruff

wswoodruff Nov 20, 2017

Author Contributor

Yup! No need to export the schema I suppose


server.path(__dirname);

const views = {
engines: { 'html': Handlebars },
path: './templates/plugin'
path: __dirname + '/templates/plugin'

This comment has been minimized.

Copy link
@devinivy

devinivy Nov 20, 2017

Member

Why this change?

This comment has been minimized.

Copy link
@wswoodruff

wswoodruff Nov 20, 2017

Author Contributor

This might've slipped through when trying to write tests based off of others. I can remove it!

Copy link

jedireza left a comment

I haven't taken a test drive yet but I did leave a couple small questions.

register: function (server, options) {

return server.register(Vision);
server.register({

This comment has been minimized.

Copy link
@jedireza

jedireza Nov 20, 2017

Should we be using awaiting the return of this? (seen later in other call sites as well)

await server.register(...

This comment has been minimized.

Copy link
@wswoodruff

wswoodruff Nov 20, 2017

Author Contributor

I've asked around about this, "what's the difference between return await [Promise] and return [Promise]? And ljharb in the Node.js Slack community said they're identical in what they do, only returning an await causes an unnecessary wrapper. Do you know if there's a difference?

This comment has been minimized.

Copy link
@jedireza

jedireza Nov 21, 2017

I see. That's good to know.

Do you know if there's a difference?

Nope. I'm new to async/await in general.

This comment has been minimized.

Copy link
@devinivy

devinivy Nov 21, 2017

Member

I find it's generally easier to remove cognitive load by simply awaiting anything async– only think about the underlying promises when really necessary. For what it's worth, in a lot of hapi code you'll find return await somePromise. Unless perf is terribly important, that seems most consistent.

This comment has been minimized.

Copy link
@wswoodruff

wswoodruff Nov 21, 2017

Author Contributor

Good to know!!

}
});

return server.route({

This comment has been minimized.

Copy link
@jedireza

jedireza Nov 20, 2017

I'm curious why you're using return here. I don't see a reference in the docs about needing to return anything from a plugin's register function. (seen later in other call sites as well)

@wswoodruff

This comment has been minimized.

Copy link
Contributor Author

wswoodruff commented Nov 20, 2017

Thanks so much for the review guys!

@thebinarypenguin

This comment has been minimized.

Copy link

thebinarypenguin commented Dec 16, 2017

I'm excited about the ability to pass view manager options at registration time, but I'm curious what does that have to do with multiple registrations?

@wswoodruff

This comment has been minimized.

Copy link
Contributor Author

wswoodruff commented Dec 16, 2017

https://github.com/hapijs/hapi/blob/master/API.md#-await-serverregisterplugins-options
mentions that once: true ... 'Cannot be used with plugin options.'

So just to allow plugin options loL ¯\(ツ)

@thebinarypenguin

This comment has been minimized.

Copy link

thebinarypenguin commented Dec 17, 2017

@wswoodruff Gotcha. Thanks for the plugin, and preemptive thanks for this new feature. 🎉

@wswoodruff wswoodruff changed the title Multiple registrations Pass manager options at registration time Dec 24, 2017
return parent.plugins.vision;
}
// This fingerprint is set when using the 'onRequest' server ext
if (this.request.route.fingerprint === '/#') {

This comment has been minimized.

Copy link
@wswoodruff

wswoodruff Dec 24, 2017

Author Contributor

I'm curious what others think of using the fingerprint /# for detecting when using the 'onRequest' server extension.

When using the toolkit view decorator inside the onRequest server extension, it seems like this.request.route.realm is set to the root server every time.

wswoodruff added 2 commits Dec 24, 2017

parent = parent.parent;
// 'this' is the plugin/server that registered the extension

This comment has been minimized.

Copy link
@devinivy

devinivy Dec 24, 2017

Member

this should be the toolkit itself. If h.view() is called in an extension then h.realm will be the realm in which the extension was created.

I'm pretty sure you want to always use h.realm and not h.request.route.realm here. Imagine writing a plugin that has its own manager and registers some extensions (but no routes). When calling h.view() in one of those extensions I think you would expect to know only about your manager/templates, and not about other managers/templates registered in a different plugin (i.e. a separate plugin that registers routes).

This comment has been minimized.

Copy link
@wswoodruff

wswoodruff Dec 24, 2017

Author Contributor

Hm, now I can't remember why I wrote it grabbing the realm from the route.

@wswoodruff

This comment has been minimized.

Copy link
Contributor Author

wswoodruff commented Dec 24, 2017

Thanks for the reviews guys! I'm hoping to merge this to master soon after the holidays, gonna let it marinate again for a little bit, not a whole month this time tho loL

@wswoodruff wswoodruff added this to the 5.3.0 milestone Dec 28, 2017
wswoodruff added 2 commits Dec 29, 2017
@wswoodruff

This comment has been minimized.

Copy link
Contributor Author

wswoodruff commented Dec 29, 2017

It's time! =P

@wswoodruff wswoodruff merged commit 6f1db29 into master Dec 29, 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
@wswoodruff wswoodruff deleted the multiple-registrations branch Dec 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.