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

app.use('/:param', paramApp) not supported #696

Closed
alexgorbatchev opened this issue Jun 5, 2011 · 13 comments · Fixed by #1935
Closed

app.use('/:param', paramApp) not supported #696

alexgorbatchev opened this issue Jun 5, 2011 · 13 comments · Fixed by #1935

Comments

@alexgorbatchev
Copy link

It seems that a use case where an app would be mounted onto a URL with arguments in not currently supported.

@tj
Copy link
Member

tj commented Jun 6, 2011

nope, that would just be a regular route, aka app.get('/:param/whatever/other/stuff', ...), this can be annoying (though personally I find it clean and explicit), so things like express-namespace or express-resource can help, but mounting itself doesn't accept params

@alexgorbatchev
Copy link
Author

I think this represents an integral feature for RESTful app design, ie a pattern of /:resource_id/action. In this case, mounting an app as a resource seems more natural than not. I can give a few examples from just our apps:

/users/:user_id/action
/:page_id/action
/tags/:tag_id/action

and so on. We are currently using

require('./users')(app)
require('./pages')(app)
require('./tags')(app)

but the following would not only be more elegant, but useful

app.use('/users/:user_id', require('./users'))
app.use('/:page_id', require('./pages'))
app.use('/tags/:tag_id', require('./tags'))

What do you think?

@tj
Copy link
Member

tj commented Jun 6, 2011

of course, but you start with the basics, allowing you to build up to anything. Starting with a high-level api is not the way to go, which like I mentioned is where express-resource comes into play etc, at it's core it's nothing more than a callback for a http method/url

@alexgorbatchev
Copy link
Author

Thank you for express-resource reference, I didn't know about it. Will check it out.

@jed
Copy link

jed commented Mar 13, 2012

this issue just bit me pretty hard. i expected app.use([String], ...) to have the same behavior as the router methods.

@tj
Copy link
Member

tj commented Mar 13, 2012

it's something we could consider for the next majors. Once node has the stuff in Connect's patch.js I wanted to consider removing .use() and the dispatcher from Connect, moving that into Express would allow Connect's components to work with any node app, then .use() could be part of Express and utilize some of the routing code if we need to. There's a lot we could clean up, app.get('/foo', cb) == app.use(match('GET', '/foo', cb)), app.use('/mount-point', cb) == app.use(mount('/mount-point, cb)) blah blah

@cjroth
Copy link

cjroth commented Feb 17, 2014

any updates here? it would be useful to have this working.

@rlidwka
Copy link
Member

rlidwka commented Feb 22, 2014

Same question, I need to mount an existing application to a variable mount point.

I suppose I could go with app.all(...) and reimplement req.url-rewriting logic, but native support would be nice

@defunctzombie
Copy link
Contributor

can you guys provide an example showing why you need to mount the app under the variable mount point and not just put the variable inside the app? Also, please try this on master and see if it works there using a Router.

@jonathanrdelgado
Copy link

+1 for use-case, can't wrap my head around a reason for this.

@jonathanong
Copy link
Member

I don't think it'll work anyways since every route overwrites the parameters.

@cjroth
Copy link

cjroth commented Feb 23, 2014

Here's an example: I have my API broken down into subapps (each resource is basically a subapp) so in my main app.js I have app.use('/farms' require('./lib/farms')); but then a farm resource has subresources like fields, so in /lib/farms/index.js I'd like to include app.use('/fields', require('./fields)); so that my lib directory can look like this:

/lib
  /farms
    index.js
    /fields
      index.js
  /another-top-level-resource
  // etc

This way I can auto-load the farm resource in the farms module and then hand it over to the fields module with the farm already loaded, and the fields can handle the rest of the request as if /fields was a top level resource. Right now my /fields routes are registered in /lib/farms like this: app.get('/farms/:id/fields/:field_id', fn); but it would be much cleaner to register them in /lib/farms/fields like this: app.get('/fields/:field_id', fn); and /lib/farms would have already have added the req.farm property.

@defunctzombie
Copy link
Contributor

Here is a sample app that fails against master when you try to navigate to /farms/123/fields

var express = require('express');

// fields get mounted under /farms/:id/
var fields = new express.Router();
fields.param('id', function(req, res, next, id) {
    req.field = {
        farm: req.farm,
        id: id,
        name: 'foobar'
    };
    next();
});
fields.get('/', function(req, res, next) {
    res.json([]);
});
fields.get('/:id', function(req, res, next) {
    res.json(req.field);
});

// farms is mounted under /farms
var farms = new express.Router();
farms.param('id', function(req, res, next, id) {
    req.farm = {
        id: id,
        name: 'foobar'
    };
    next();
});
farms.use('/:id/fields', fields.middleware);
farms.get('/:id', function(req, res, next) {
    res.json(req.farm);
});

var app = express();
app.use('/farms', farms.middleware);
app.listen('3000');

// call to /farms/123/fields does not work

I think this should work and don't see a reason why the parameter handling is being done the way it is currently per route versus at the router level. I will be taking a look at this.

@defunctzombie defunctzombie reopened this Feb 23, 2014
@expressjs expressjs deleted a comment from sespinoza-dev Jun 27, 2019
@expressjs expressjs locked and limited conversation to collaborators Jun 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants