Skip to content

Implement route conditional middleware. #8

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mattcollier
Copy link
Contributor

This takes inspiration from:
https://stackoverflow.com/a/19337607
https://stackoverflow.com/a/40418065

This works, but I'm looking for ideas for how to make it performant. see todos.

@davidlehn
Copy link
Member

I don't really understand the server._router part so pardon if I'm missing something. I think setting this up with the main config could lead to confusion.

I'll present a slightly different solution that would be implemented as setting up per-route options at the route definition site and use that with conditional middleware wrappers. The setup could be like what bedrock-docs annotate does with methods, routes, and doc options. I think checks will have to happen every request, so middleware could be used to always get any defined route options. Would certainly need to optimize the route option lookup. I'm not sure how this compares to the router stack scan code. It would only be run once per request in case you had multiple conditionals.

bedrock-express setup would always setup route options on the request and could have conditional middleware wrappers:

      // always setup per-route options
      server.use(_routeOptions)
      // ...
      if(bedrock.config.express.useSession) {
        server.use(_routeConditionalMiddleware({
          default: true,
          test: req => req.routeOptions.useSession !== false,
          fn: api.middleware['express-session'](bedrock.config.express.session)
        }));
      }
      // ...

_routeOptions would run every request and setup a request var based on req.route.path and req.route.methods(?). Could probably optimize so that best case overhead is one req.route.path lookup in a map.

function _routeOptions(req) {
  if(...) {
    req.routeOptions = ...;
  }
}

The conditional code can be generic:

function _routeConditionalMiddleware({default, test, fn}) {
  return function(req, res, next) {
    if((!req.routeOptions && default) || (req.routeOptions && test(req))) {
      fn(req, res, next);
    } else {
      next();
    }
  }
}

bedrock-express would need a method+route+options api simlar to bedrock-docs.annotate for .all, .get, .post, etc:

api.routeOptions.XXX = function(route, options) { ... };

Then apps could setup per-route options as needed:

const brExpress = require('bedrock-express');
brExpress.routeOptions.get('/foo/:id', {useSession: false});
app.get('/foo/:id', function(req, res, next) { ... });

A drawback is if you override a route you'd have to know how to override the options too.

This all does involve some plumbing and overhead but seems like a fairly generic solution to get options to middleware that's run early.

Unfortunate that the express API doesn't just have a way for routes to run early middleware to set flags for later middleware.

@mattcollier
Copy link
Contributor Author

@davidlehn to add to the last thing you said, you mentioned somehow using req.route. That's not a thing in this global middleware. Beyond that, I don't really know what you're suggesting. So, maybe an alternate PR proof-of-concept is in order? Again, this method does work but definitely has some performance penalties. Adding routes to a blacklist in the config does not seem that unusual to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants