router
router copied to clipboard
pillarjs
Add Router events
The following events were added:
-
handlestart- emitted when the router starts to handle the router stack. -
layer- emitted when a layer is matched by the router. It returns the matched layer. -
handleend- emitted when the router has completed handling the router stack.
This feature should help developers handle requirements as described in https://github.com/expressjs/express/issues/2501, https://github.com/expressjs/express/pull/3100, and similar.
Example of usage with Express:
app.router.on('handlestart', function () {
console.log('------- %d -------', Date.now())
})
app.router.on('layer', function (layer) {
console.log(layer)
})
app.router.on('handleend', function () {
console.log('------- %d -------', Date.now())
})
@dougwilson @danieljuhl @evanshortiss \o/
@hacksparrow this is nice. I guess "layer" would be the event I'm looking for, but where can I associate that to the request itself here? For example, being able to do the below, would be nice, but needs req to be injected:
app.router.on('layer', function (layer, req) {
req.matchedLayers = req.matchedLayers ? req.matchedLayers.concat([layer]) : [layer]
})
The end goal being that I can have a middleware like so:
function routeLogger (req, res, next) {
res.on('finish', function () {
// write to analytics, or log route
console.log(req.matchedLayers); // Maybe do some formatting here to get the actual paths joined
});
}
We will also need to discuss how the events are bubbled (or not). For example, would a user be required to attach an event listener on every single router (and of course, understand the list of all their routers) or should this use a form of event bubbling between routers, similar to the DOM?
@evanshortiss great suggestion. "handlestart" and "handleend" would need the request context too.
@dougwilson excellent feedback. I will implement a bubbling version as well for further thoughts on the feature.
@evanshortiss the event handlers have been updated to the following:
app.router.on('handlestart', function (req, router) {
})
app.router.on('layer', function (layer, req, router) {
})
app.router.on('handleend', function (req, router) {
})
That looks solid to me. Should provide plenty of context for introspection. Nice work! 👍
@hacksparrow why add the router to the event? Isn't that available within the event handler as the context (this)?
So, just for the record, the signature now is:
app.router.on('handlestart', function (req) {
})
app.router.on('layer', function (layer, req) {
})
app.router.on('handleend', function (req) {
})
Does anyone have any thoughts on the event names? I'm neutral on them :)
@dougwilson @crandmck I have updated the PR according to your suggestions.
For the record, the signature of the layer event listener now is:
app.router.on('layer', function (req, layer) {
})
Anything blocking this? I would love to replace what I have been doing in middleware with these new events.
There are still issues I wrote above unaddressed, with the biggest that it is reaching into the internals of Node.js events which is unlikely to keep working in the future.
Ahh, sorry @dougwilson I didn't open the "outdated diffs". I completely agree with the comments, and am anxiously awaiting this merge :) Thanks for the good work @hacksparrow!!
Also, what ever happened with the whole event bubbling discussion? I feel like I've mentioned it before, and @hacksparrow said he would look into it, but never really anything seemed to happen there. So what is the thoughts on that? Does that make it easier to accomplish whatever the goals of this PR are or not?
@dougwilson I haven't made significant progress on event bubbling yet. Will update once done. In the mean time, it'd be great to hear the community's opinion about a bubbling and non-bubbling version.
@dougwilson @hacksparrow just to be clear, by bubbling you mean bubble to an upper "app" (express) instance? So for submounted apps, the event will bubble to the parent app and so on?
@evanshortiss bubbling within the routers (independent or the one that comes with Express) hierarchy. We were thinking along lines of the browser DOM events.
TBH I think you have to do bubbling for layer events, if you don't I think the behavior will be unexpected. That being said, multiple start and end events seem problematic, or atleast overly complex. I see two options on that:
- "Bubbled" events get prefixed, or a different name altogether
-
handlestart/handleendevents don't bubble, but layer events still do
To address the cases that I would use this for right away, here are two I can think of:
- Unbind events on the front-end when the NEXT
handlestartevent happens - Monitoring performance metrics for each layer
So for number one, having multiple start events for one "run" of the router stack, would mean I would have to track it myself. Possible, obviously, but it would be nicer if the design compensated for this and gave me a simple way.
For number two, each "layer", IMO should include all sub routers. Thus requiring bubbling.
So I am circling back on this, and after re-thinking it I do not think anything other than the layer event should be included. Here are my thoughts:
-
There are complexities this introduced, as discussed above in detail. None of which have simple resolutions.
- Bubbling: the semantics of this are hard to get right for start and end, but pretty clear for layer
- Handle End: Currently it is the response end, but this can already be handled with the
on-finishedpackage, and the router end has little to no value.
-
The feature's we have all mentioned can be solved in other ways:
- Original Route/Matched Route: #34 solves this in a much better way
- Metrics: If you actually care to get accurate metrics you should be capturing the start time sooner than this, and as mentioned above,
handleendis an alias foronFinished(res, cb). - Logging: Because #34 gets you all the info you need, you can then just log in a middleware at the end of your stack
That all being said, the one thing we cannot do with ease is track stats and metrics on individual layers. Which is why I propose that this change just to:
-
layerstart: Same as the currentlayerevent is now -
layerend: Added once next is called after matching a layer
So I created #56 to illustrate and keep the discussion going.
Hi guys! so is this still happening? :)
If it is, as I said in my last comment, I would hope we just go with the layer events. That being said, I am not sure even this is the best way forward, and I have an idea which has been bubbling about adding a decorator mechanism which would allow for much more flexibility and also superseded the need to add these events at all.
I have left open my other PR since it might be good, but I think this PR should be closed. If no one disagrees (@dougwilson or @hacksparrow) then I will close this in a week (reminder already set to revisit).
Ok, no comments. But @dougwilson it looks like I am not an owner on this org so cannot close this. 🤷♂️
@dougwilson It looks like just adding me to the tc group is not enough to be able to close issues. Seems like there must be some settings to make that work, but I am not sure.
Sorry @wesleytodd about that. I will check it out and make sure your permissions are in order. You should have the ability do to this, so I will check why here shortly 👍
Finally coming back around here. This is superseded by https://github.com/pillarjs/router/pull/56 IIRC. Closing this, and assessing if that could possibly land for router@2 and express@5.