Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Passing path as an array runs callbacks before parameter callbacks #1549

Closed
chrisfosterelli opened this Issue Mar 19, 2013 · 16 comments

Comments

Projects
None yet
6 participants

On Express 3.1, passing an array of paths will have them run before the parameter callback. I have a parameter set via a app.param('dog') call that sets req.dog, and while this code works:

  app.all('/api/v1/dog/:dog/s', function(req, res, next) {
    console.log(req.dog);
    next();
  });
  app.all('/api/v1/dog/:dog/b', function(req, res, next) {
    console.log(req.dog);
    next();
  });

This code crashes with an undefined error:

  app.all(['/api/v1/dog/:dog/s',
           '/api/v1/dog/:dog/b'], function(req, res, next) {
    console.log(req.dog);
    next();
  });

Basic placing on console.log() calls shows that the parameter is being ran after the app.all call that is passed the array, while it runs before in the first scenario.

Member

jonathanong commented Apr 14, 2013

an array of paths is not supported nor documented by express, so this is a feature request, not a bug.

hallas commented Apr 15, 2013

When does it crash? When you visit either route? or when you start express?

Owner

tj commented Apr 15, 2013

arrays got in quite a while back but they were never documented, I would definitely prefer to remove support for that, there's not really a good reason to support them

hallas commented Apr 15, 2013

i don't really see a reason to support them

Either way, if you do decide to keep support for them I think it would be best to at least document it or make it behave more like expected. Currently, passing an array doesn't run the parameter handlers before the array callback.

My use for them is setting up middleware that runs on many routes (checking auth and ownership of objects), but it requires that the parameter handler be called first.

It's understandable why it would also be removed too, but I think it shouldn't be a "half complete" support. It should either removed, or works like it intuitively should in my opinion.

@hallas it crashes when I visit the route.

hallas commented Apr 15, 2013

Obviously if you pass an array of routes, they should all be processed, but there is a problem because their layout changes how req.params is populated, obviously not a problem if they contain the same amount and named ":xyz" parameters, but if they dont, then what?

Maybe if an array is passed it simply becomes an alias for calling the app.verb function 1..n times?

Owner

tj commented Apr 15, 2013

yeah it's because we just join in the regexp, really we should be applying the route twice like you mention. I'll add a deprecation warning and phase it out

hallas commented Apr 15, 2013

fine by me 👍

Is there a best way to handle several routes with one function?
Now I handle them with array:
app.get(['/u', '/u/:login/*', '/upload'], handler);
After deprecation i must do:
app.get('/u', handler);
app.get('/u/:login/*', handler);
app.get('/upload', handler);

Is some more elegant way exists?

Owner

tj commented May 1, 2013

what's inelegant about that? it's simple, not like that's a common use-case, terse != elegant. Alternatively you can "rewrite" req.url, so with a middleware something like:

app.use(rewrite('/u', '/upload'))
app.use(rewrite('/u/:login/*', '/upload'))
...

blah blah but that's not really any different

Taking this opportunity, I want to ask :)

app.get('/u', appMainHandler);
app.get('/u/*', appMainHandler);

Is there a way to write it in one route?

@klimashkin There is also no reason you can't make it an array and loop through it.

var routes = [ 
  '/u', 
  '/u/:login/*', 
  '/upload'
];

routes.forEach(function(route) {
  app.get(route, handler);
});

Although arguably that's just as messy.

Owner

tj commented May 2, 2013

@klimashkin app.get('/u*', callback) should work

Contributor

reqshark commented Jun 3, 2013

Owner

tj commented Jun 3, 2013

forgot to close

@tj tj closed this Jun 3, 2013

@zevero zevero referenced this issue in pouchdb/pouchdb-server Aug 12, 2013

Closed

Allowing pouchdb-server to run as middleware! #18

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