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

http: add res.req and res.redirect() #33606

Closed
wants to merge 3 commits into from
Closed

Conversation

XadillaX
Copy link
Contributor

@XadillaX XadillaX commented May 28, 2020

Add res.req and res.redirect() in http server for convenience.

Fixes: #28673

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label May 28, 2020
Add `res.req` and `res.redirect()` in http server for convenience.

Fixes: nodejs#28673
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label May 29, 2020
@jasnell
Copy link
Member

jasnell commented May 29, 2020

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.

@XadillaX
Copy link
Contributor Author

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.

@BridgeAR
Copy link
Member

It expands the API surface in a way that frameworks that build on the core apis already handle very well.

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.

@BridgeAR
Copy link
Member

@nodejs/http @nodejs/http2 PTAL

@lpinca
Copy link
Member

lpinca commented May 30, 2020

I'm also -1. I think this is outside of "core" scope.

Copy link
Member

@mcollina mcollina left a 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.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@wesleytodd
Copy link
Member

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?

Copy link
Member

@ronag ronag left a 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.

@ghermeto
Copy link

I agree with @jasnell and I believe that we should only increase the public core API where it makes most impact.

#28673 doesn't make a compelling argument that this will benefit the majority of the users (as frameworks already do it) or that the alternative (passing req as a param) is too hard.

@XadillaX
Copy link
Contributor Author

Okay, we can keep discussing then.

@XadillaX XadillaX requested a review from a team as a code owner August 10, 2020 16:07
@styfle
Copy link
Member

styfle commented Jan 18, 2021

res.req was implemented in PR #36505

@XadillaX
Copy link
Contributor Author

How about redirect()?

@styfle
Copy link
Member

styfle commented Jan 19, 2021

I would prefer redirect() that matches the Express.js API.

But I agree with the sentiment that there's not a lot of value in 1 line res.redirect(url, 302)
vs 2 lines res.statusCode=302; res.setHeader('Location',url).

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.

@bl-ue
Copy link
Contributor

bl-ue commented Jun 5, 2021

This should probably be closed as the general consensus is that it's not a good idea.

@mcollina mcollina closed this Jun 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

attach req as res.req