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

mountpath isn't always a string #57

Open
jasisk opened this issue Apr 13, 2015 · 4 comments
Open

mountpath isn't always a string #57

jasisk opened this issue Apr 13, 2015 · 4 comments
Labels

Comments

@jasisk
Copy link
Contributor

jasisk commented Apr 13, 2015

We assume it is, but it's not. Can also be an array.

@dougwilson
Copy link

Pretty much app.mountpath is just whatever the user had mounted the app on. It's basically one of three things:

  1. Just a single string. This means it was a "traditional" path app.use('/path', subApp).
  2. A RegExp object. This means it was on a regular expression app.use(/^\/path\/?/, subApp)
  3. An array that is any combination of the above two. app.use(['/path', '/path2', /^\/what(?:\/do)\/?/], subApp)

I hope this helps you, @jasisk :)

@jasisk
Copy link
Contributor Author

jasisk commented May 6, 2015

@dougwilson you've found the crux of my issue the other day. ;) Since we're here, I'll just talk about what I'm trying to do in this issue.

We use the pattern I've termed the "ephemeral app" pattern all over the place. Meddleware, kraken-js, and swaggerize-express are just three examples. I think it's a great pattern from an adoption standpoint—as far as our users are concerned, they're just mounting another piece of middleware like they've probably done thousands of other times. In return, we get access to the mountpath and parent app instance without the user having to explicitly pass them to us.

You can see how the pattern works from the code however—for completeness—it's basically these steps:

  1. Create an app instance
  2. Register a mount listener on the app instance—listener callback receives the parent app instance (that which our instance is mounted on) as the first parameter and ...
    1. Store the mountpath of the app instance (now accessible as app.mountpath)
    2. Pop our app instance off the parent app instance's router stack
  3. Return the app instance

This works great for kraken and, presumably, most other cases as well.

Where the issue comes up is in mounting other middleware on the parent app on a new mountpath. This is precisely what meddleware tries to accomplish with its route configuration.

Here's an example of how it works in the best case (string mountpath):

var meddleware = require('middleware');
var express = require('express');
var app = express();

var opts = {
  myMiddleware: {
    route: '/secure',
    module: './lib/myMiddleware'
  }
};

app.use('/data', meddleware(opts));

In this best-case scenario, the mountpath of meddleware is successfully concatenated with the route for myMiddleware, and we end up with this equivalent behavior:

app.use('/data/secure', require('./lib/myMiddleware')());

The problem is that we can't simply concat regexps. And we could certainly walk arrays and concat strings, ignoring regexps, but I wanted to find a better solution.

So my next attempt was to create a new Router, register all the middleware on that, and mount it on the parent app at mountpath. That way, we can just rely on express' route resolution, and we don't have to try to normalize the path (mountpath + route). Something like this (pseudocode):

function meddleware() {
  var app = express();
  app.on(mount, onMount);

  function onMount(parent) {
    var mountpath = app.mountpath;
    var router = express.Router();

    middlewares.forEach(function (middleware) {
      router.use(middleware.route, middleware.module);
    });

    app.use(mountpath, router);
  }

  return app;
}

This works great for everything except error-handling middleware. It falls down because of the following behavior:

var app = express();
var otherApp = express();

var router = express.Router();

var ehm = function (err, req, res, next) {
  res.send('handled');
};

var badMiddleware = function () { throw new Error(); };

app.use('/', ehm);

router.use(/* '/', */ ehm);
otherApp.use('/', router);

app.get('/', badMiddleware);
otherApp.get('/', badMiddleware);

// get '/' on `app`, get "handled"
// get '/' on `otherApp`, get `finalhandler` output

The behavior is understandable, as we discussed in expressjs/express#2633, but still throws a wrench in the best alternative I could think of for supporting the full array of mountpaths.

So ... any ideas? 😀

@dougwilson
Copy link

Would you be interested in setting up a Google Hangout or Skype meeting? If so, send me an email and we can set it up :) I haven't read through all this just yet, just wanted to throw this out there up front while I read everything here.

@jasisk
Copy link
Contributor Author

jasisk commented May 6, 2015

👍 email forthcoming. 😄

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

No branches or pull requests

2 participants