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

Lifecycle hooks on routes #2566

Closed
Marsup opened this issue May 27, 2015 · 11 comments
Closed

Lifecycle hooks on routes #2566

Marsup opened this issue May 27, 2015 · 11 comments
Assignees
Labels
Milestone

Comments

@Marsup
Copy link
Contributor

@Marsup Marsup commented May 27, 2015

What would you think of allowing the lifecycle hooks (all but onRequest obviously) to be set per route ?
The filtering I have to do to match routes in those functions just pollutes the readability of it.
Having global hooks when they're not used as such probably also is a tiny waste of resource however minor.

@Marsup Marsup added the request label May 27, 2015
@hueniverse hueniverse added this to the 9.0.0 milestone May 27, 2015
@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented May 27, 2015

I've long wanted to change the ext functionality to be per:

  • server
  • connection
  • plugin
  • route

But I am worried about the overhead of checking for all those. Right now when nothing is set it is a simple null check at each ext point. For this to work I will need to change the lifecycle array generation to actually build the array per route as well as rebuild it when extensions are added later after route config.

Maybe for 9.0.

@devinivy
Copy link
Member

@devinivy devinivy commented May 30, 2015

This would be wonderful. @hueniverse are you worried about overhead at the time of server start, or throughout the course of a single request?

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented May 30, 2015

Only care about hot code. Executing the request lifecycle is hot.

@seanrucker
Copy link

@seanrucker seanrucker commented Aug 5, 2015

Looking forward to this feature; here's my use case...

I'm using Hapi as an API Gateway. I have four clients accessing this API, each has its own set of routes. One of the clients is an Ember app which requires the API response to conform to the JSON API spec. I'm using a Hapi plugin (inspired by hapi-json-api.js) to handle some of the boilerplate code required to munge the response into something JSON API compliant. The plugin uses Hapi lifecycle hoks which unfortunately has the effect of making all my routes conform to JSON API.

@hueniverse hueniverse removed this from the 9.0.0 milestone Aug 10, 2015
@hueniverse hueniverse removed this from the 9.0.0 milestone Aug 10, 2015
@hueniverse hueniverse added this to the 10.4.0 milestone Oct 4, 2015
@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Oct 4, 2015

What would the interface look like for adding extensions to one or more routes? I got ideas but want to see what other people think.

@devinivy
Copy link
Member

@devinivy devinivy commented Oct 4, 2015

I see three options, some of which could probably work in tandem.

  1. Add an ext() method to the route public interface.

    server.lookup('route-id').ext('onPreAuth', extensionFn, options);
  2. Add an ext option to route config. Adding extension options would start to look messy.

    server.route({
        method: 'GET',
        path: '/',
        config: {
            id: 'route-id',
            handler: function (request, reply) { return reply('ok'); },
            ext: { onPreAuth: [extensionFn] }
        }
    });
  3. Add a routes option to server.ext(). This addresses ext() options and the ability to add it to multiple routes.

    server.ext('onPreAuth', extensionFn, { routes: ['route-id'] });

I think I favor the first and third options, possibly in tandem, since it seems like core should keep server extensions programmatic rather than configured. A plugin (i.e. mrhorse) could implement something like the second option. One thing I don't like about option three on its own is that it requires that the route have an id. If ext() were also part of the route public interface, extensions could be added to routes without an id. That seems important– imagine I want to write a plugin that adds route extension points in some arbitrary way to any route in the routing table.

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Oct 5, 2015

I do like 1 although 2 would make it easier if adding to multiple routes, maybe a tag system can help where you tag routes with keywords and select on them to add extensions to all of them

@devinivy
Copy link
Member

@devinivy devinivy commented Oct 5, 2015

If collections of routes were selectable by tag, that would be neat. Most important is that extensions can be added to routes programmatically and arbitrarily (not requiring a tag or id). I do like the syntax of 2 since it's descriptive, but I don't think it's flexible enough. We have an issue in mrhorse to support that type of route configuration (mark-bradshaw/mrhorse#23), so I can nearly guarantee that there will be a tool in the hapi ecosystem similar to the second option if hapi doesn't implement it.

@Marsup
Copy link
Contributor Author

@Marsup Marsup commented Oct 5, 2015

I also prefer version 2. It would be nice to also be able to have the same format globally, much like we inherit properties from the server currently.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Oct 5, 2015

For now I added support for option 2 and a new sandbox option for plugins. This allows you to specify extensions at the route level in the route config, at the connection level for all routes, or at the plugin level for all routes added by the plugin.

What is missing is being able to define a route selection criteria and apply an extension to that, like an extension for all GET requests. But I need to think about it some more so this will do for now.

@lock
Copy link

@lock lock bot commented Jan 9, 2020

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants