search

chilts / connect-pgclient

Connect middleware to manage Postgres connections.
Other
7 stars 0 forks source link

Subscribe to res 'finish' event instead of redefining res 'end()' event #2

Open andreypopp opened 11 years ago

andreypopp commented 11 years ago

It's better to subscribe to an finish event on res instead of monkey-patching its end() method.

chilts commented 11 years ago

I just copied what connect-session does. In some ways I don't mind how it does it and it kinda makes sense to me. It's just middleware that needs to run on the way out of the middleware stack rather than something orthogonal to it as you're proposing.

So, I'm not sure what to do here. Unless there are +1's I'll probably leave it. An example patch would be welcome so I can see how you'd do it. :)

Cheers, Andy

chilts commented 11 years ago

Ah, I see how you do it here : https://github.com/andreypopp/domain-context/blob/master/lib/index.js#L104-L119

andreypopp commented 11 years ago

It seems that connect-session was implemented before the response became a stream and even streams were introduced into the node's stdlib. But you are probably right — there's no point to fix things which are not broken.

Related connect PRs aren't yet accepted — https://github.com/senchalabs/connect/pull/552

dcworldwide commented 7 years ago

@chilts Did you refactor the code with this? Cheers

chilts commented 7 years ago

Hi @dcworldwide - hmm, I can't quite remember since it was so long ago, however, I'm almost certain I didn't do anything. Does this issue still exist? Does anyone actually use this package? :)