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

dependency as part of plugin attributes #2803

Closed
danielb2 opened this issue Sep 30, 2015 · 21 comments
Closed

dependency as part of plugin attributes #2803

danielb2 opened this issue Sep 30, 2015 · 21 comments
Assignees
Labels
feature New functionality or improvement

Comments

@danielb2
Copy link
Contributor

Currently server.dependency is used to set package dependency and the callback is necessary in order to define, for example, routes that rely on other plugins.

exports.register = function (server, options, next) {

    server.dependency(['vision'], internals.after);
    return next();
};

exports.register.attributes = {
    name: 'test',
    version: '1.0.0'
};

internals.after = function (server, next) {

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

        return reply.view('index');
    }})
    return next();
};

This is a common pattern now that vision is extracted from core.

Could dependencies handled in the plugin attribute make this easier? This would indicate to hapi to not try to load the plugin until dependencies have been met.

exports.register = function (server, options, next) {

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

        return reply.view('index');
    }})

    return next();
};

exports.register.attributes = {
    name: 'test',
    version: '1.0.0',
    dependency: ['vision']
};
@devinivy
Copy link
Member

There already is a dependencies plugin attribute that's supported in hapi. But it doesn't register any after callbacks– it just enforces that the dependencies are present when the server initializes.

Relatedly, I'm working on a module that will reorder a list of plugins so that they load in the correct order based upon the dependencies attribute.

@danielb2 danielb2 reopened this Sep 30, 2015
@danielb2
Copy link
Contributor Author

I shouldn't open tickets when I'm tired. I'm specifically looking for something that makes sure they're registered first. Your module sounds awesome

@devinivy
Copy link
Member

Check here soon: https://github.com/devinivy/hodgepodge

@danielb2
Copy link
Contributor Author

Any reason to implement it this way as opposed to it being part of core?

@devinivy
Copy link
Member

This just isn't the way hapi plugin dependencies are architected, and it would add complexity into hapi core to allow for deferred plugin registration. @hueniverse has been suggesting for a while that tools be created to assist with the bare-bones plugin registration that hapi provides, so maybe that's the direction the community oughtta head if we want to see solutions to plugin-registration woes.

@hueniverse
Copy link
Contributor

You are basically asking for the entire register method to be inside an after callback.

@danielb2
Copy link
Contributor Author

danielb2 commented Oct 1, 2015

Yes, for now I've been following this pattern :

exports.register = function (server, options, next) {

    server.dependency(['vision'], internals.register(options));

    return next();
};

internals.register = function (options) {

    return (server, next) {

        // logic
        return next();
    }
}

@devinivy
Copy link
Member

devinivy commented Oct 1, 2015

hapi plugin dependency resolution– seems to be ready now: https://github.com/devinivy/hodgepodge . Definitely has some limitations– please give feedback. @danielb2 I used your pattern above as part of an example.

@danielb2
Copy link
Contributor Author

danielb2 commented Oct 1, 2015

great timing as my laptop suddenly stopped working and my current work was lost. I'll give feedback soon :)

@AdriVanHoudt
Copy link
Contributor

Cool plugin @devinivy! Not sure why you added attributes.hodgepod since you only delete it https://github.com/devinivy/hodgepodge/blob/master/lib/index.js#L36 but this feels more like how hapi should deal with registering plugins

@devinivy
Copy link
Member

devinivy commented Oct 1, 2015

I added it so that a plugin can require that it be registered using hodgepodge. If you register it without deleting that attribute, hapi will throw since it's not a valid attribute. Which is intended– you want to guarantee that the attribute is consumed.

Edit
Btw thanks, @AdriVanHoudt! Worth adding, it would be harder for hapi to support this because hapi doesn't require that all plugins be registered at the same time. Hodgepodge does require that. Plugins aren't deferred to wait for their dependencies (which is what hapi would probably have to do)– they're just reordered. This relies directly on the fact that hapi registers plugins in series.

@AdriVanHoudt
Copy link
Contributor

Ah like that I see, was not obvious :P And hmm fair point... thanks for the explanation, didn't think about registering at different times

@danielb2
Copy link
Contributor Author

danielb2 commented Oct 2, 2015

@devinivy is it necessary to register the plugin with hodgepodge before being able to use it? The documentation isn't clear, and my setup uses a manifest file in JSON format which is read in using glue with confidence. Hodgepodge would need a solution for that if this is the case.

@devinivy
Copy link
Member

devinivy commented Oct 2, 2015

The way it works is that you give it a list of plugin registrations as would be passed to server.dependency(). It returns a reordered list of the same plugins, which you can then pass directly to server.dependency(). Use with glue will be hard to support, as glue's configuration is not an array, so it technically has no definitive order; and glue manifests don't contain the dependency information. Glue could incorporate hodgepodge into an option and use it internally. Do you have any other ideas? It would be nice if it were compatible with glue / rejoice.

Ex. of how to use hodgepodge,

var plugins = Hodgepodge([
  pluginA, // pluginA.register.attributes.dependencies === 'pluginB'
  pluginB 
]);

// Now plugins looks like [pluginB, pluginA]
// This ordering respects pluginA's dependency on pluginB

server.register(plugins, function (err) {/* ... */});

Edit
@danielb2 is this example in the docs not enough? Maybe I should put it up higher in the docs. https://github.com/devinivy/hodgepodge#registering-plugins

@danielb2
Copy link
Contributor Author

danielb2 commented Oct 2, 2015

I think up higher would be good, and it could be shown in with vs without for further clarity. The last bit is a bit like an afterthought: "btw, you also have to do this to make it work", especially since it came after the without example.

For our setups we do all use glue/rejoice because we have so many plugins we have to load. We used to have a big async.waterfall setup, but moved away from that in favor of rejoice.

@devinivy
Copy link
Member

devinivy commented Oct 2, 2015

Thanks– let me know if the update to the readme is an improvement. Feel free to move this over into an issue on the hodgepodge repo since this issue is closed.

Edit Oops– realized it's not closed.

@hueniverse
Copy link
Contributor

I am not going to add this to core at this point. It adds significant complexity because the only place where it can perform this sort in a consistent and useful way is in server.initialize() but at that point the plugin lose access to after() and dependency(). This means a plugin using such metadata will not be allow to use some of the server APIs in register().

Since this functionality already exists, it is not worth the complexity and edge cases to solve it in core.

@hueniverse hueniverse self-assigned this Oct 2, 2015
@devinivy
Copy link
Member

devinivy commented Oct 3, 2015

Out of curiosity @hueniverse, do you think a tool like hodgepodge is worthwhile?

@hueniverse
Copy link
Contributor

I think if your plugin setup is so complex that you need it, you might want to rethink your mess. It makes sense as part of a bigger tool chain.

@danielb2
Copy link
Contributor Author

danielb2 commented Oct 3, 2015

@hueniverse my feelings is that while following the pattern I have is fairly straight forward, it's already more messy than I'd like. Is there a cleaner way that I've missed?

@devinivy I think hodgepodge didn't really gain me anything over what I'm doing already unless I can have it working more seemleessly, especially with Glue. IOW, it's just as messy.

@hueniverse
Copy link
Contributor

That's the process. Cleaner will require tooling to basically auto generate that code.

@Marsup Marsup added feature New functionality or improvement and removed request labels Sep 20, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New functionality or improvement
Projects
None yet
Development

No branches or pull requests

5 participants