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

Added Route.prototype.verbs #2615

Closed
wants to merge 6 commits into from
Closed

Added Route.prototype.verbs #2615

wants to merge 6 commits into from

Conversation

B3rn475
Copy link

@B3rn475 B3rn475 commented Apr 3, 2015

This method allows to register a set of verbs in just one call and allows to define a default route for all the Others.

This is different from using all, because the default route is added only to the verbs for which there is not a defined one.

@B3rn475 B3rn475 changed the title Added Route.prototype.register Added Route.prototype.verbs Apr 3, 2015
@dougwilson
Copy link
Contributor

Hi! I kinda think the name "verbs" sounds weird, but IDK. Otherwise my only comment right now is can you paste in (or even better, add a full app to the examples for with tests) here a real-world use-case adding this solves that is not easy or impossible with the current Express functionality?

@B3rn475
Copy link
Author

B3rn475 commented Apr 4, 2015

The use-case I'm thinking about is related to the error 405 Method Not Implemented.
In order to implement that now I need to do this.

route
  .get(function (req, res) { res.end(); });
  .post(function (req, res) { res.status(405).end(); });
  .put(function (req, res) { res.status(405).end(); });
  .delete(function (req, res) { res.status(405).end(); });

With verbs instead

route
  .verbs({
     get : function (req, res) { res.end(); }
     default: function (req, res) { res.status(405).end(); }
  });

I cannot use .all because otherwise I will not be able to call next without calling it.
What do you think about this?

P.S. verbs is just because it is used to add verbs. If you find a better name for me is the same ;-)

@dougwilson
Copy link
Contributor

If 405 is the only reason, then I would probably reject this, because we want to add 405 handling already (and a PR is welcome). Do you have a non-405 use-case?

@B3rn475
Copy link
Author

B3rn475 commented Apr 4, 2015

It is a way to add more than one verb at a time.
It even allows to return an entire route configuration (all the verbs) as one single object. It can simplify route management.

app.route('/users').verbs(require('./routes/users));

I know it is syntactic sugar.

@rlidwka
Copy link
Member

rlidwka commented Apr 4, 2015

It won't simplify route management, 'cause people usually do this anyway:

app.use('/users', require('./routes/users'));

I don't see any tasks the alternate syntax would solve better.

@B3rn475
Copy link
Author

B3rn475 commented Apr 4, 2015

To do what you are suggesting I need to build a full application and return it if I want to handle verbs.

--- Messaggio originale ---

Da: "Alex Kocharin" notifications@github.com
Inviata: 04 aprile 2015 17:16
A: "strongloop/express" express@noreply.github.com
Cc: "Carlo Bernaschina" b3rn4@hotmail.it
Oggetto: Re: [express] Added Route.prototype.verbs (#2615)

It won't simplify route management, 'cause people usually do this anyway:

app.use('/users', require('./routes/users'));

I don't see any tasks the alternate syntax would solve better.


Reply to this email directly or view it on GitHub:
#2615 (comment)

@rlidwka
Copy link
Member

rlidwka commented Apr 4, 2015

Router, not application. Those should be pretty lightweight.

@B3rn475
Copy link
Author

B3rn475 commented Apr 4, 2015

@dougwilson what do you suggest for 405 management? I can be interested in implementing it.
Something like this?

app
  .route('/')
  .get(function (req, res, next) { next(); })
  .post(function (req, res, next) { next(); })
  .none(function (req, res) { res.status(405).end(); });

Or something more explicit?

@dougwilson
Copy link
Contributor

Just a route constructor option. There is an open issue with this discussion, including API thoughts. Feel free to continue 405 discussion in that issue rather than this PR.

@B3rn475 B3rn475 closed this Apr 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants