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

Deprecate app.all('/*', fn) #2245

Closed
dougwilson opened this issue Jul 17, 2014 · 6 comments
Closed

Deprecate app.all('/*', fn) #2245

dougwilson opened this issue Jul 17, 2014 · 6 comments

Comments

@dougwilson
Copy link
Contributor

This is an idea to deprecate and display a message when someone adds the route .all('/*', fn) (part of the preparation for #2173). The * at the end syntax is going away, and this specific case with .all is a prime thing to start yelling at people about now in 4.x since it makes no sense. I can't imagine a reason they wouldn't want .use(fn), except for mis-understanding.

@jonathanong
Copy link
Member

a whole list of practices we could think of that makes us go "WTF r u doing?" would be nice as error messages i think. can't cite any examples though

@defunctzombie
Copy link
Contributor

I think this goes down a potentially bad road of tons of random code to catch cases we don't normally care about but with the downside of potentially preventing someone from experimenting with something.

@dougwilson
Copy link
Contributor Author

I'm not sure about @jonathanong 's comment, but the intent of the original issue here is because the new path-to-regexp does not support the trailing * syntax, and we need to come up with a way to add deprecation warnings for this stuff, but can only add a warning if there is actually an alternative--this specific .all case does have one.

@dhigginbotham
Copy link

There's probably one or two valid use cases for .all('/*',...) but for the most part I really think that .use(...) is always a better solution -- however leave .[methodType]('/*',...) as is, so you don't have to involve method logic inside of your middleware. However, even then the argument is a toss-up because I really feel like it's a facade for .use(...) in the end... But, maybe not. 👍

@dougwilson
Copy link
Contributor Author

There's probably one or two valid use cases for .all('/*',...) but for the most part I really think that .use(...) is always a better solution

There is no use-case, unless you want every OPTIONS request to your server to list every single method in the Allow response.

@dougwilson
Copy link
Contributor Author

This was a stop-gap idea and is no longer necessary since I have a better way to go about adding deprecation messages to old path syntax. People are free to shoot themselves in the foot using app.all incorrectly :)

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

4 participants