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

New API for specifying http method #43

Open
wesleytodd opened this issue May 19, 2016 · 8 comments
Open

New API for specifying http method #43

wesleytodd opened this issue May 19, 2016 · 8 comments

Comments

@wesleytodd
Copy link
Member

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?

@Twipped
Copy link

Twipped commented May 19, 2016

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?

@wesleytodd
Copy link
Member Author

wesleytodd commented May 20, 2016

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 () {});

@dougwilson
Copy link
Contributor

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.

@wesleytodd
Copy link
Member Author

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?)

@dougwilson
Copy link
Contributor

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

@wesleytodd
Copy link
Member Author

wesleytodd commented May 20, 2016

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...

@LinusU
Copy link

LinusU commented Sep 28, 2016

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...

@palanik
Copy link

palanik commented Jul 28, 2017

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.

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

5 participants