Add Router events #49

hacksparrow · 2016-10-24T10:41:41Z

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 expressjs/express#2501, expressjs/express#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/

evanshortiss · 2016-10-25T18:36:10Z

@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]
})

evanshortiss · 2016-10-25T18:45:30Z

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
  });
}

dougwilson · 2016-10-25T18:50:59Z

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?

hacksparrow · 2016-10-26T15:52:21Z

@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.

hacksparrow · 2016-10-27T17:12:18Z

@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) {

})

evanshortiss · 2016-10-27T17:46:53Z

That looks solid to me. Should provide plenty of context for introspection. Nice work! 👍

dougwilson · 2016-10-27T18:09:26Z

@hacksparrow why add the router to the event? Isn't that available within the event handler as the context (this)?

hacksparrow · 2016-10-27T18:25:12Z

@dougwilson err yes! I will remove it from the params. Thanks.

hacksparrow · 2016-10-27T18:46:03Z

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) {

})

dougwilson

Additional to the other comments, any thoughts about the DOM-like event bubbling?

dougwilson · 2016-11-02T22:44:12Z

index.js

@@ -186,6 +192,9 @@ Router.prototype.handle = function handle(req, res, callback) {
  req.baseUrl = parentUrl
  req.originalUrl = req.originalUrl || req.url

+  // trigger the "beginning of route handling" event
+  if (events && routerEvents[0] in events) self.emit('handlestart', req)


Is routerEvents[0] in events actually saving any time? I'm not sure if accessing _events and using in on it is really a good long term API to use with event emitters.

Agreed. I decided on this to keep the naming convention "flexible". Will come up with a better alternative.

dougwilson · 2016-11-02T22:45:06Z

index.js

@@ -269,6 +278,9 @@ Router.prototype.handle = function handle(req, res, callback) {
      return done(layerError)
    }

+     // trigger the "layer found" event
+    if (events && routerEvents[1] in events) self.emit(routerEvents[1], layer, req)


If we swap req and layer, all three events will conveniently start with req as the first argument.

This is a good suggestion 👍

dougwilson · 2016-11-02T22:47:36Z

test/router.js

+        var server = createServer(router)
+
+        router.on('handlestart', function (req) {
+          assert(IncomingMessage.prototype.isPrototypeOf(req))


I would just check for certain properties, perhaps add a request header and try to read it here, rather than checking the prototype specifically. I know there are a bunch of people interested in better browser support, so if there is no reason to hurt the support, I would err on the side of that.

dougwilson · 2016-11-02T22:49:39Z

test/router.js

+        var counter = 0
+        router.on('layer', function (layer, req) {
+          counter++
+          if (handlers.length === counter) done()


I'd recommend using the after function for this type of tracking, which will throw when invoked too much.

dougwilson · 2016-11-03T00:12:02Z

Does anyone have any thoughts on the event names? I'm neutral on them :)

crandmck · 2016-11-03T00:13:59Z

Please add some docs for this to the README.

The following events were added: 1. `handlestart` - emitted when the router starts to handle the router stack. 2. `layer` - emitted when a layer is macthed by the router. It returns the matched layer. 3. `handleend` - emitted when the router has completed handling the router stack. I am sure very sure about the name of the events, feedback are most welcome.

hacksparrow · 2016-11-10T10:38:26Z

@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) {

})

crandmck · 2016-11-11T19:31:23Z

Thanks, docs LGTM.

wesleytodd · 2016-11-13T19:33:54Z

Anything blocking this? I would love to replace what I have been doing in middleware with these new events.

dougwilson · 2016-11-13T20:37:08Z

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.

wesleytodd · 2016-11-13T20:41:58Z

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!!

dougwilson · 2016-11-13T20:39:19Z

index.js

+
+  // trigger the "end of route handling" event
+  if (events && 'handleend' in events) {
+    res.once('finish', function () {


What is the purpose of adding the finish event listener? Especially because there are no tests around all the edge cases this will cause (for example, are we saying that if the response is already finished before the handleend, our events is not supposed to fire?)

dougwilson · 2016-11-13T20:40:24Z

index.js

@@ -164,6 +168,7 @@ Router.prototype.handle = function handle(req, res, callback) {
  var self = this
  var slashAdded = false
  var paramcalled = {}
+  var events = self._events


Let's not peak into the internals of Node.js if it's not required, and I see no reason this is required. If there is, please enlighten me :)

dougwilson · 2016-11-13T20:40:58Z

index.js

@@ -20,6 +20,7 @@ var mixin = require('utils-merge')
 var parseUrl = require('parseurl')
 var Route = require('./lib/route')
 var setPrototypeOf = require('setprototypeof')
+var EventEmitter = require('events').EventEmitter


This should probably be alphabetical like the rest.

dougwilson · 2016-11-13T20:42:02Z

README.md

+
+### 2. `layer`
+
+This event is emitted when a route layer is matched.


Can you define "route layer" in the docs?

dougwilson · 2016-11-13T20:43:24Z

README.md

+})
+```
+
+Here is a complete example of using router events in an Express 5 app.


This should not be part of the handleend section.

dougwilson · 2016-11-13T20:45:14Z

README.md

+Here is a complete example of using router events in an Express 5 app.
+
+```js
+var express = require('express')


Can the example be a real example instead of just a bunch of console log lines? I.e. demonstrate the use case people will use this for. People love to copy paste, and this is not something they would do that on.

dougwilson · 2016-11-13T20:46:35Z

test/router.js

@@ -983,8 +984,153 @@ describe('Router', function () {
      .expect(200, 'saw GET /bar', done)
    })
  })
+
+  describe('events', function () {
+


These tests have a very inconsistent style from all other tests if you can clean them up at some point :)

dougwilson · 2016-11-13T21:06:14Z

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?

hacksparrow · 2016-11-17T00:06:39Z

@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.

evanshortiss · 2016-11-17T00:15:04Z

@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?

hacksparrow · 2016-11-17T00:21:34Z

@evanshortiss bubbling within the routers (independent or the one that comes with Express) hierarchy. We were thinking along lines of the browser DOM events.

wesleytodd · 2016-11-17T00:23:07Z

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/handleend events don't bubble, but layer events still do

wesleytodd · 2016-11-17T01:04:17Z

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 handlestart event 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.

wesleytodd · 2017-03-25T19:15:11Z

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-finished package, 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: Store matched routes in request #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, handleend is an alias for onFinished(res, cb).
- Logging: Because Store matched routes in request #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 current layer event is now
layerend: Added once next is called after matching a layer

So I created #56 to illustrate and keep the discussion going.

fcolella · 2019-03-06T20:49:04Z

Hi guys! so is this still happening? :)

wesleytodd · 2019-05-31T21:59:58Z

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).

wesleytodd · 2019-06-07T18:02:50Z

Ok, no comments. But @dougwilson it looks like I am not an owner on this org so cannot close this. 🤷‍♂️

wesleytodd · 2019-06-09T03:55:47Z

@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.

dougwilson · 2019-06-09T03:58:52Z

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 👍

wesleytodd · 2024-03-16T18:44:47Z

Finally coming back around here. This is superseded by #56 IIRC. Closing this, and assessing if that could possibly land for router@2 and express@5.

dougwilson added ideas pr labels Oct 25, 2016

hacksparrow force-pushed the router-events branch 3 times, most recently from 5756cca to 585eeb0 Compare October 27, 2016 16:28

hacksparrow force-pushed the router-events branch from 585eeb0 to 40c292f Compare October 27, 2016 18:42

hacksparrow mentioned this pull request Nov 2, 2016

Express TC meeting 11-02-2016 expressjs/discussions#42

Closed

dougwilson reviewed Nov 2, 2016

View reviewed changes

evanshortiss mentioned this pull request Nov 7, 2016

Add originalRoute like originalUrl expressjs/express#2501

Open

hacksparrow force-pushed the router-events branch from 40c292f to f95996f Compare November 10, 2016 09:19

hacksparrow force-pushed the router-events branch from f95996f to 79f8afa Compare November 10, 2016 09:31

dougwilson mentioned this pull request Nov 13, 2016

Add tracing capabilities to Express expressjs/express#2437

Closed

dougwilson suggested changes Nov 13, 2016

View reviewed changes

wesleytodd mentioned this pull request Mar 25, 2017

Router events (WIP Attempt 2) #56

Open

wesleytodd mentioned this pull request Feb 22, 2019

Allow req.on('next', (err) => {...}); #78

Closed

dougwilson force-pushed the master branch from bb3b1fe to 5fa7c9c Compare January 24, 2021 01:32

dougwilson force-pushed the master branch 2 times, most recently from 7e90c9e to 8643ec6 Compare May 19, 2021 16:51

wesleytodd closed this Mar 16, 2024


		### 2. `layer`

		This event is emitted when a route layer is matched.

Add Router events #49

Add Router events #49

Conversation

hacksparrow commented Oct 24, 2016 • edited by dougwilson Loading

evanshortiss commented Oct 25, 2016

evanshortiss commented Oct 25, 2016

dougwilson commented Oct 25, 2016

hacksparrow commented Oct 26, 2016 • edited Loading

hacksparrow commented Oct 27, 2016 • edited by dougwilson Loading

evanshortiss commented Oct 27, 2016 • edited Loading

dougwilson commented Oct 27, 2016

hacksparrow commented Oct 27, 2016

hacksparrow commented Oct 27, 2016 • edited by dougwilson Loading

dougwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacksparrow Nov 3, 2016 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougwilson commented Nov 3, 2016

crandmck commented Nov 3, 2016 • edited Loading

hacksparrow commented Nov 10, 2016

crandmck commented Nov 11, 2016 • edited Loading

wesleytodd commented Nov 13, 2016 • edited Loading

dougwilson commented Nov 13, 2016

wesleytodd commented Nov 13, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougwilson commented Nov 13, 2016

hacksparrow commented Nov 17, 2016

evanshortiss commented Nov 17, 2016

hacksparrow commented Nov 17, 2016

wesleytodd commented Nov 17, 2016 • edited Loading

wesleytodd commented Nov 17, 2016

wesleytodd commented Mar 25, 2017 • edited Loading

fcolella commented Mar 6, 2019

wesleytodd commented May 31, 2019

wesleytodd commented Jun 7, 2019

wesleytodd commented Jun 9, 2019

dougwilson commented Jun 9, 2019

wesleytodd commented Mar 16, 2024

hacksparrow commented Oct 24, 2016 •

edited by dougwilson

Loading

hacksparrow commented Oct 26, 2016 •

edited

Loading

hacksparrow commented Oct 27, 2016 •

edited by dougwilson

Loading

evanshortiss commented Oct 27, 2016 •

edited

Loading

hacksparrow commented Oct 27, 2016 •

edited by dougwilson

Loading

hacksparrow Nov 3, 2016 •

edited

Loading

crandmck commented Nov 3, 2016 •

edited

Loading

crandmck commented Nov 11, 2016 •

edited

Loading

wesleytodd commented Nov 13, 2016 •

edited

Loading

wesleytodd commented Nov 17, 2016 •

edited

Loading

wesleytodd commented Mar 25, 2017 •

edited

Loading