Add a method to remove middleware #696

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

JonDum commented Nov 27, 2012

I came across a situation where I needed to remove middleware after the server has started. Alas, there wasn't any method to do this.

My use case consists of allowing the changing, adding, removing of Static directories from persistent user settings. Yes, I could write some fancy request handlers to do this, but why reinvent the wheel when the Static middleware does everything I need perfectly? Making middleware more effective > reinventing the wheel, no?

I didn't really know what to name the function so I went with unuse(). Add middleware with use(), remove it with unuse().

Also, my preferred method would have been to return an id when you call use() so that you can use that to simply remove that specific middleware. That would require some heavy refactoring and would break chaining. Instead, unuse() removes all middle of the same type at the same route. I couldn't think of any instances where you would want

Member

tj commented Nov 27, 2012

why not just "wrap" the static() middleware any apply logic, this would be much more efficient. there has been a few requests for this so far but none that cant be solved by applying a tiny bit of logic within preceding middleware or wrapped middleware

here's a contrived example:

app.use(setting('serve files', static('public')))

function setting(name, fn){
  return function(req, res, next){
    res.user.enabled(name, function(err, yes){
      if (yes) {
        fn(req, res, next);
      } else {
        next()
      }
    })
  }
}

JonDum commented Nov 27, 2012

I'm really not a fan of returning an anonymous function like that. This causes unnecessary function allocations that when garbage collected could cause a hiccup. If that function is used frequently, this could kill performance during gc in a high load server. Also, that still doesn't provide any method to remove middleware from the stack, just ignore it.

Sure, this method doesn't require changing the api, but I think it's better to manage the stack array directly rather than use a workaround like this.

If you don't like that remove parameter, I could rewrite unuse to not call call use, then copy some of the logic from unuse into use, but there'd be duplicate code.

Member

tj commented Nov 27, 2012

?.. no it really doesn't... one function is going to have absolutely no affect

removing middleware by name is hacky, you can have more than one static() for example

Contributor

TooTallNate commented Nov 27, 2012

This causes unnecessary function allocations that when garbage collected could cause a hiccup. If that function is used frequently, this could kill performance during gc in a high load server.

Benchmarks?

JonDum commented Nov 27, 2012

Contrived example:

https://gist.github.com/4157610

Yes, this is extremely unrealistic. It just shows that returning anonymous functions does have an impact. Regardless I still stand by

better to manage the stack array directly rather than use a workaround like this.

Member

tj commented Nov 27, 2012

when you have 1000000 of them of course.. it is memory after all, even our ./node_modules alone has over 80000 functions, this is an insane micro-optimization

JonDum commented Nov 27, 2012

I know haha. That is why I don't stand by that point. Nate wanted a benchmark, so...

Anyways, if a user wrote a whole bunch of middleware to use for their own project, they'll have to use these Setting wrappers for all of the ones they want to remove (which still doesn't really remove them) instead of just providing a method to remove the middleware directly?

It's just odd. Why promote using middleware as a means to structure and clean code, then not allow users to control the stack of middleware? :(

Member

tj commented Nov 27, 2012

the main issue is there's no unique reference to any given middleware that is added, and swapping things out at runtime is a slightly odd pattern, especially since middleware are 99.9999% not user-specific, you end up doing queries and user logic within the req/res cycle anyway

Member

tj commented Nov 28, 2012

closing for now

tj closed this Nov 28, 2012

We need something like this to use with the vhost middleware, since our application needs some kind of "service nginx/apache reload", so there's no downtime when you want to add a vhost to the stack. What about this use case? Would you consider adding the logic directly into the unuse() (or whatever name you think is better) method, to avoid having to change use() interface and logic?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment