router icon indicating copy to clipboard operation
router copied to clipboard

New API for specifying http method

Open wesleytodd opened this issue 9 years ago • 8 comments

Starting with a discussion in #41, we need to come up with a new api for specifying the http method[s] for a handler. Here are a few examples:

  1. router.methods('GET', 'POST').use('/', function () {});
  2. router.use(['GET', 'POST'], '/', function () {})
  3. router.route('/').methods('GET', 'POST').handler(function () {})
  4. router.route(['GET', 'POST'], '/').handler(function () {})

All of these are fairly consistent from the current API in my opinion, despite being breaking changes in a few cases. Numbers 1 & 3 are not breaking changes AFAIK since they are just adding new methods. Thoughts?

wesleytodd avatar May 19 '16 19:05 wesleytodd

I think I'm leaning towards number 3. It's the most consistent with the way handlers are currently bound to routes.

Is there a reason methods() wouldn't just take the function as the last argument instead of needing the separate handler() call?

Twipped avatar May 19 '16 21:05 Twipped

I am also leaning to number 3, I actually modeled it a bit after a Go library that I thought had a nice api, gorilla mux. But after looking at these again and writing some use case examples I think it would be better to do something like:

  1. router.route('/').methods('GET', 'POST').use(function () {})

Then use could operate just like it always has, accept a sub path as a first arg, and multiple middleware as the rest of the arguments. This also only adds one new method, methods, keeping the api simple.

The reasoning for not including the handler function in the methods call, IMHO, is that it mixes two pieces of functionality together that do not strictly belong together. Also, it would have to take multiple middleware to be comparative to the current use syntax. Lastly I think it would enable some different styles of route declarations that some people might like, for example, I could see a use case where you could do something like this:

var router = new Router();

var gets = router.methods('GET');
gets.use(function cacheThings () {});
gets.use('/', function () {});
gets.use('/:foo', function () {});

var posts = router.methods('POST');
posts.use('/:foo', function () {});
posts.use('/:bar', function () {});

EDIT: For reference here is the current way of doing the above example:

var router = new Router();
router.get(function cacheThings () {});
router.get('/', function () {});
router.get('/:foo', function () {});
router.post('/:foo', function () {});
router.post('/:bar', function () {});

wesleytodd avatar May 20 '16 15:05 wesleytodd

So the only issue I have with that syntax is the method name "use". That method name has always been used to mean only the path prefix will be matched, and it will not be an exact path match (thus it's notable absence from the .route() stuff). I don't think we should use ".use()" for any usage that matches an exact path.

dougwilson avatar May 20 '16 15:05 dougwilson

Ok, I can see that for sure. Are you recommending going back to a new method called handler? (Or another method with the same functionality?)

wesleytodd avatar May 20 '16 15:05 wesleytodd

I'm not sure. I've been racking this around my head since you opened the issue, on those APIs. .handler I guess makes sense, but does not sounds good, haha, and .use has a current expected behavior from seeing that method name around Express.js and the greater ecosystem, so using it here will likely be confusing.

I've been trying to think of it this way: If I had the following code

app.route('/user/:id')
.get(function () { ... })
.delete(function () { ... })
.put(function () { ... })

What would it look like if hypothetically these were all custom methods? I' don't have any good answers off the top of my head yet, which is the great part about having a discussion issue :D

dougwilson avatar May 20 '16 16:05 dougwilson

For other references, here are how that golang lib I mentioned works: http://www.gorillatoolkit.org/pkg/mux

I think your example could look something like:

app.route('/user/:id')
  .methods('GET').handler(function () {})
  .methods('DELETE').handler(function () {})
  .methods('PUT').handler(function () {})

Another option, and IMO worse idea, is this:

app.route('/user/:id')
  .methods({
    'GET': function () {},
    'DELETE': function () {},
    'PUT': function () {},
  })

The reason I think it is "worse" is that it is a huge departure from the current api. But I figured it is just another discussion point...

wesleytodd avatar May 20 '16 17:05 wesleytodd

At my current job we have our own wrapper over express which exposes an api like so:

app.route(method, path, handler)

e.g.

app.route('get', '/greet/:name', (res, res) => {
  res.send(`Hello ${req.params.name}`)
})

I think it looks really neat and it have been working great for us...

LinusU avatar Sep 28 '16 19:09 LinusU

What do you think of this:

Along the lines of all(), introduce some() with additional parameter(s) for methods.

router.route('/')
.some(['GET', 'POST'], function (req, res, next) {
  ...
});

or

router.route('/')
.some('GET', 'POST', function (req, res, next) {
  ...
});

Technically, all() can be extended to support some() and backward compatible.

palanik avatar Jul 28 '17 17:07 palanik