-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
http: add res.req and res.redirect() #33606
Conversation
Add `res.req` and `res.redirect()` in http server for convenience. Fixes: nodejs#28673
I appreciate the work on this but I think I'm -1 on this. It expands the API surface in a way that frameworks that build on the core apis already handle very well. |
Hmmmm, IMHO, the point is that not only one framework do that. |
I don't think that's an argument against this. It would probably be good to get feedback from maintainers of some of the frameworks @wesleytodd @dougwilson @mcollina. |
@nodejs/http @nodejs/http2 PTAL |
I'm also -1. I think this is outside of "core" scope. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am -1 as well because these core methods will be shadowed by express or will be useless as the other frameworks do not use them.
Generically, I regret having such a rich API for http.
8ae28ff
to
2935f72
Compare
I am not completely opposed, but this would probably be something we should discuss in conjunction with the frameworks. If this was implemented in core it would be overridden in express and other frameworks. Most users would not get the direct benefit until the frameworks wrapped it instead of their implementations, and in this case express has a different method signature. We attempt to be a light wrapper around core, this would be an exception and would require a breaking change for us to align with core. All that said, I think we should have a clear conversation about why we think these things which basically all frameworks implement (even if they do it well) are not in core. Maybe this would be something to add to a future @nodejs/web-server-frameworks agenda? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather not increase the API surface and leave this to user land.
I think this is a good topic for the @nodejs/web-server-frameworks agenda.
Okay, we can keep discussing then. |
|
How about |
I would prefer But I agree with the sentiment that there's not a lot of value in 1 line And what should the default status code even be? 301, 302, 307, 308? Since this PR has no upvotes, I would say we don't need it in core. |
This should probably be closed as the general consensus is that it's not a good idea. |
Add
res.req
andres.redirect()
in http server for convenience.Fixes: #28673
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes