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

Please add app.route('foo').use() #1980

Open
nakedible opened this issue Mar 16, 2014 · 31 comments
Open

Please add app.route('foo').use() #1980

nakedible opened this issue Mar 16, 2014 · 31 comments

Comments

@nakedible
Copy link

For Express 4.x, documentation says "Using app.route() is a recommended approach to avoiding duplicate route naming and thus typo errors." However, since all() and use() are different, it would be nice to be able to call use() on route objects as well, to avoid duplicate route naming.

@nakedible
Copy link
Author

Suggested tags: 4x, Feature Request, Routes.

@defunctzombie
Copy link
Contributor

We decided against this because .use carries a specific meaning with what it does to req.path

@nakedible
Copy link
Author

Doesn't route do the same? That is, strip the path segment from the url? Or is there some more I'm not seeing?

In any case, does this means that your recommended usage is:

app.use('/app', middleware);
app.route('/app')
  .get(function (req, res) { ... });

or

app.route('/app')
  .all(middleware)
  .get(function (req, res) { ... });

Does the latter make a difference in OPTIONS behavior or something else?

@jonathanong
Copy link
Member

no, app.route() doesn't strip the path segment from the url. only .use() does. up to @defunctzombie if he wants to support this though. it could make the internal code simpler if all middleware used .route() internally, but i'm not sure i see a use-case for this.

@defunctzombie
Copy link
Contributor

I need to check on the OPTIONS behavior but I would lean towards making sure .all doesn't affect OPTIONS for .route or app.all() but that might break some other assumptions people have made (or backwards compat) so it could be a no-go.

The second approach is what I would recommend but it could depend on your reliance on req.path inside the middleware.

@nakedible
Copy link
Author

Well, at the very least, when enabling all debugging, using .all will add 24 separate handlers. That doesn't seem reasonable for simply using a middleware.

The way I see it is simply that there are three types of handlers on the incoming requests:

  • Handlers that simply skip requests not matching path prefix
  • Handlers that skip requests not matching path prefix and strip prefix from requests matching path prefix
  • Handlers that match a certain METHOD and resolve path containing wildcards, params, etc.

These three types should be usable in all situations.

@nakedible
Copy link
Author

Yeah, sorry, route() does not strip the path, I confused it with Router - I thought those two were the same thing, with route('/path') just calling app.use('/path', new Router()) and returning the generated instance.

@defunctzombie
Copy link
Contributor

I think we should change .all to simply add middleware and not affect options responses. Yes, it will be slightly different than app.all, but whatever.

@jonathanong
Copy link
Member

I think we should change .all to simply add middleware and not affect options responses. Yes, it will be slightly different than app.all, but whatever.

+1

@danschumann
Copy link

I'm a fan of the following syntax, since i keep my apps very modularized:

(note: often times a sub app will handle most of it's own routing, so this much indentation wouldn't be necessary irl)

app

  .route('/library')
    .use(middleware.auth.hasLibraryCard)
      .route('/books')
        .get(controllers.books.index)

        .get('/:book_id',
          controllers.books.show)

        .route('/overdue')
          .get(controllers.overdueBooks.index)
          .post(controllers.overdueBooks.create)
          .end() // this would be nice to help in chaining

        .post(controllers.books.create)
        .end() 

      .route('/shelves')
        .route('/:shelf_id')
          .use(middleware.loadShelf) // to req.locals or something
          .get(controllers.shelves.show)
          .post(controllers.shelves.update)
          .end()
        .end()
      .end()
    .end()

  .route('fire_station')
    // etc....

cheers!

@jonathanong
Copy link
Member

easier to make a module that does that for you, then see if people are interested in it. adding it to express means additional bloat.

plus, i hate that .end() stuff. and the ridiculous amount of nesting.

@nijikokun
Copy link

plus, i hate that .end() stuff. and the ridiculous amount of nesting.

👍

@dougwilson dougwilson added the 4.x label May 31, 2014
@dougwilson
Copy link
Contributor

I think we should change .all to simply add middleware and not affect options responses. Yes, it will be slightly different than app.all, but whatever.

FYI route.all never did affect the options behavior, so there is nothing to change there. route.use is still useful in situations where you want to add a middleware that is expecting the path to be stripped (like static and such).

@defunctzombie
Copy link
Contributor

Sounds like a 5.x feature maybe. Along with possibly ripping out the routing system into a module finally.

@dougwilson
Copy link
Contributor

Adding route.use should be trivial for 4.x, right? I was thinking of adding it for the 4.5 release.

@dougwilson
Copy link
Contributor

@nakedible after re-reading your original posts, it is clear you are confusing app.all behavior with route.all behavior: app.all adds 24 handlers and affects OPTIONS, but route.all does not and only adds a single handler.

@dougwilson
Copy link
Contributor

Anyway, a route.use() that just operates like route.all() but strips the path does not seem useful, because of course the path will always be /. BUT I think route.use() would be useful if it would make the middleware only invoked if there was a handler for the incoming method. This would make it very useful for adding middleware to your route, but if your route only did GET and POST, your middleware wouldn't even be invoked when a TRACE came in on your route for no reason.

Thoughts? @defunctzombie @jonathanong

@dougwilson dougwilson mentioned this issue Jul 14, 2014
5 tasks
@defunctzombie
Copy link
Contributor

I think having a route.use() that strips the path could be interesting. It would allow you to built up trees of routes minimizing repeating the same route string. I support either adding it and maintaining the strip path semantics or not adding it.

@dougwilson
Copy link
Contributor

@defunctzombie you're thinking a signature of route.use(path, fn), right (where path defaults to '/')?

@defunctzombie
Copy link
Contributor

route.use(fn) because the path is built in already

@dougwilson
Copy link
Contributor

Well, I don't see how that would really build trees, then? Do you have an example? My only example I can think of is

var userRouter = express.Router()
userRouter.route('/:id')
.get(getUser)
.put(updateUser)
app.route('/users')
.get(getUsers)
.post(createUser)
.use(userRouter)

But really all those handle under the route could just be under '/' in the router...?

@dougwilson
Copy link
Contributor

Actually, the above would never work, because .route(path) matches up the full path, so /user/1 would never come through there.

@defunctzombie
Copy link
Contributor

Yea, actually I think you are right in that it should support (path, fn) if it supports anything at all.

@dougwilson
Copy link
Contributor

Well, I just realized that route(path) does a full match on the path, so like path to use would essentially only match downwards, like app.route('/users/:id').use('/users', fn) would work, but app.route('/users').use('/users/:id', fn) would not work or something.

@defunctzombie
Copy link
Contributor

Right, the .use would be path after stripping away the route's path. Then it could strip away any additional path you pass as well.

@dougwilson
Copy link
Contributor

Right, the .use would be path after stripping away the route's path.

What I'm saying is that the route's path is actually a match-to-the-end match, so app.route('/users') would not match /users/1 and such, which seems like a .use() under that would just never match anything to me...

@defunctzombie
Copy link
Contributor

It seems that having this feature is useless then?

@dougwilson
Copy link
Contributor

It seems that having this feature is useless then?

Right. So the only use I could come up with was the one above, which would let a user do this:

app.route('/users')
.use(bodyParser.json())
.get(getUsers)
.post(createUser)

and the bodyParser.json middleware would not even be invoked for a PUT /users. It's super easy to implement, but I'm not sure how much of a value it would be. It would basically be the short-hand of doing

app.route('/users')
.get(bodyParser.json(), getUsers)
.post(bodyParser.json(), createUser)

@kokujin
Copy link

kokujin commented Aug 6, 2014

-1 for ".use()" in routing. Its a terminology that confuses people, "use" for middleware and such and nothing else

@StreetStrider
Copy link

Why app.route() doesn't have use() when Router() instance has? What's the difference and what is the proper manner to mount subapp/router over app.route() instance? Looks like all has another meaning so I need use to mount whole subapp.

@gireeshpunathil
Copy link
Contributor

looks like a nice discussion around router abstraction, but with no forward path, or convergence. Is closing the right thing to do here?

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

No branches or pull requests

10 participants