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

Add support for conditional loading of middleware #2545

Closed
wants to merge 1 commit into from
Closed

Add support for conditional loading of middleware #2545

wants to merge 1 commit into from

Conversation

gustavnikolaj
Copy link

I have a need to be able to install middleware based on the value of configuration.

It could be done with express-noop like this:

app.use(condition ? middleware : require('express-noop')());

That is doing it's job, but it is wasteful, as there is no need to register any middleware if the condition is false. It's only being evaluated up front any way.

It could also be done by using an if sentence.

var app = express();

if (condition) {
    app.use(middleware);
}

But it is annoying when you are nesting express applications to build structures:

var app = express();
var app2 = express();

if (condition) {
    app2.use(middleware);
}

app2.use(bar)

app.use(foo)
   .use(app2);

With this PR you could use .useif() instead of use and make it all much more convenient and readable:

var app = express();

app
  .use(foo)
  .use(express()
    .useif(condition, middleware)
    .use(bar));

I hope that I made my motivation and use-case clear, and that the test coverage and code style lives up to your expectations. If there is anything that I can improve, I'll be more than happy to change it.

@gustavnikolaj
Copy link
Author

I have been using this technique already and have grown quite fond of it.

For reference, this is the code I have used to monkey patch it into express:

require('express/lib/application').useif = function (condition) {
    if (condition) {
        var args = Array.prototype.slice.call(arguments, 1);
        return this.use.apply(this, args);
    }
    return this;
};

@Twipped
Copy link

Twipped commented Feb 12, 2015

This is the exact use case that I created pitstop for. It's also what Issue #2241 was hoping to address.

@gustavnikolaj
Copy link
Author

@ChiperSoft, This is supposed to only have affect when initially registering the middleware. The use of next(...) described in #2241 seems more like a runtime thing to me. I might misunderstand it...

I see how pitstop can solve the same problem, but it has a wider scope, as I guess that the function that you can pass to pit.condition() will be evaluated at runtime too. My .useif() suggestion is like pit.condition() but static...

@Twipped
Copy link

Twipped commented Feb 12, 2015

Oh, this is just about deciding if the middleware should even be added to that stack at startup?

Yeah, no, use an if statement. 👎

@gustavnikolaj
Copy link
Author

I extended the patch than I'm using and split it out into it's own module. https://github.com/gustavnikolaj/express-use-if

@dougwilson
Copy link
Contributor

Closing, mostly because we feel it should be solved in user-land.

@dougwilson dougwilson closed this Jun 19, 2015
@gustavnikolaj gustavnikolaj deleted the feature/useif branch September 28, 2015 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants