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

attach req as res.req #28673

Closed
ianstormtaylor opened this issue Jul 13, 2019 · 7 comments
Closed

attach req as res.req #28673

ianstormtaylor opened this issue Jul 13, 2019 · 7 comments
Labels
feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem.

Comments

@ianstormtaylor
Copy link
Contributor

ianstormtaylor commented Jul 13, 2019

I'd like to propose attaching the IncomingMessage (req) object in an HTTP server to the ServerResponse (res) object:

res.req = req

Because it is often the case that when creating a response you need to access information related to the request itself.

(If you read on you'll notice that all of the popular frameworks do this under the covers, and that Node's internal logic also uses this connection, but that it's not available for smaller utilities in userland.)


The current state of things.

As things stand right now, for HTTP response-related logic you often end up needing to have access to the req object too to handle a variety of edge-case situations, which leads to either verbosity or inconsistency, and either way… confusion.

For example, imagine writing a sendJson helper:

function sendJson(res, status, json, options = {}) {
  const { spaces = 2, pretty = true } = options
  let string

  if (pretty) {
    string = JSON.stringify(json, value, spaces)
  } else {
    string = JSON.stringify(json)
  }

  res.statusCode = status
  res.setHeader('Content-Length', Buffer.byteLength(string))
  res.setHeader('Content-Type', 'application/json; charset=utf-8')
  res.end(string)
}

All seems fine. But this doesn't account for HEAD requests that should not include a body. To fix this you'd have to add (req, ...) to the signature, which is unintuitive. This is a frequent issue with HTTP-related utilities, and it part of the reason why you end up having to reach for a framework, instead of seeing a proliferation of small, useful HTTP functions.

All the major userland frameworks do this already.

This has already been established as a common practice in userland, because of how often you need to tweak a response based on the request.

Often times the req is needed only for HTTP-specific plumbing edge cases (like checking HEAD or OPTIONS requests) that are not immediately obvious to the user's mental model. Which is why it's helpful if HTTP logic can access it implicitly for the edge cases where it's required.

Most of these frameworks also do the reverse, attaching the response to the request for parallelism/consistency.

Technically, you can already do the reverse.

You can actually already do the reverse—accessing the res object from the req without a framework, by using the req.connection._httpMessage property. Now, this isn't exactly encouraged seeing as it's an undocumented property, but it shows that it's not out of the question to create these kinds of links between these two objects.

Why not always pass req into helpers?

A counter-argument would be to say that req should just be passed into any helper that needs to use properties from it. This sounds reasonable at first, but the issue with this approach is that it leads to mismatch with user expectations.

The solutions become inconsistent because access to the req isn't always required. For example, consider a few different helpers for setting the correct headers on a response:

  • A simple setHeader(res, name, value) helper doesn't need the req, because it just sets the value to whatever was passed in.
  • But a setCors(res, options) helper would actually need to be setCors(req, res, options) because you need to determine if the request used the OPTIONS method.
  • A setAuthenciate(res, scheme, options) helper also doesn't need the req object.
  • But a setRequestId(res, options) would need the req to default the ID in case it was already set.

The exact nature of the examples is irrelevant—they could be implemented in many different ways. But the point is that access to req is required in certain situations when building a response, but not in others. Which leads to APIs that are inconsistent in signature between (res, ...) and (req, res, ...), causing confusion.

And it's often required for low-level, edge-case HTTP behaviors that most users of these modules ideally don't need to think about, because they are HTTP plumbing.

You might argue that then all response-related helpers should include (req, res, ...) to keep it consistent. But if you consider things like sendJson(res, status, json, options), adding an extra (req, ...) to every arguments list becomes tedious. Especially so for helpers that don't use the req at all, and are just forced to add it for consistency for others. It feels pretty dumb to have to do getBasicAuth(req, res) just because the library needs consistency to handle ~10–20% cases.

This awkwardness is why the major HTTP frameworks for Node have all included a way to access the request from the response themselves. They know that for users it's much simpler to think about working on either the request or the response. But that edge cases require having access to both objects at once.

Node's core uses the "reference" itself internally.

When creating the res object in the first place, Node's core has access to req. And it actually does some of the same exact things that are being advocated for here, by calculating some internal logic for the response based on the request.

if (req.method === 'HEAD') this._hasBody = false;

node/lib/_http_server.js

Lines 155 to 156 in a013aa0

this.useChunkedEncodingByDefault = chunkExpression.test(req.headers.te);
this.shouldKeepAlive = false;

node/lib/_http_server.js

Lines 713 to 716 in a013aa0

if (req.headers.expect !== undefined &&
(req.httpVersionMajor === 1 && req.httpVersionMinor === 1)) {
if (continueExpression.test(req.headers.expect)) {
res._expect_continue = true;

These are all internal situations where specific response-related logic is being calculated by the request metadata and attached to res for future use. It just happens that Node already has the req in scope when creating the res. But it would be nice for userland to be able to do the same sorts of things with the res only.

This would open up new userland abilities.

Right now HTTP-related pieces of userland are almost exclusively handled by large monolithic frameworks like Express or Hapi. This is partly because there doesn't exist a simpler, Lodash-like set of utilities for handling HTTP requests in a simple functional way. Being able to link req and res would make building an intuitive library like this much easier.

Not only does this allow for people to opt-out of using a framework. But, more importantly, it allows other utilities to be built in a framework-agnostic way.

You could imagine…

image

…a world where the frameworks themselves aren't necessary. This would be helpful Lambda-like situations, or for simply adding extra HTTP-aware logic to adjacent domains, or just for keeping things extremely simple while prototyping. A lot of different use cases would benefit.

A good example of this is Vercel's built-in HTTP modifications. These kinds of behaviors are happening all over the place in the FaaS world, and only further fragment Node's HTTP handling. It would be amazing if non-monolithic solutions to this problem could thrive.


In summary...

Attaching res.req is a simple addition to Node's core that would make writing HTTP servers without needing a framework easier, which is especially helpful for writing framework-agnostic utilities, or in Lambda-like environments where you need smaller dependencies, or in performance-critical situations where framework overhead is harmful. All major userland HTTP frameworks already all do this.

It would also make it possible for userland to step up and create a nice set of framework-agnostic helper modules (eg. setCors, or setRequestId), without forcing them to have more confusing APIs than their framework-coupled counterparts—since at the moment they can't benefit from the link.

I hope that all makes sense. I think this would a small change, but one that unlocks a lot of value in userland at many layers of the stack.

Thanks for reading!

@mscdex
Copy link
Contributor

mscdex commented Jul 13, 2019

You might argue that then all response-related helpers should include (req, res, ...) to keep it consistent. But if you consider things like sendJson(res, status, json, options), adding an extra (req, ...) to every arguments list becomes tedious. Especially so for helpers that don't use the req at all, and are just forced to add it for consistency for others.

If you're that concerned about the number of parameters (growing or otherwise stable), then use an object for everything else besides req and res.

Attaching res.req would be a simple addition to Node's core that would ease writing HTTP servers without extra frameworks. (Especially helpful in Lambda-like situations, or performance-critical situations) where framework overhead is harmful. All major userland HTTP frameworks already all do this.

Why would you need a framework just to do res.req = req?


I don't see the point of this considering how easy this is to do in userland. Besides simply doing res.req = req in the first request handler, you can also supply your own class/constructor to extend node http response (and request) objects which could in addition to setting this.req = req in your constructor, you could also add all of your helper methods to the prototype. Then you don't have to worry about having to do req.res = req in your request handling code.

@ianstormtaylor
Copy link
Contributor Author

ianstormtaylor commented Jul 13, 2019

Why would you need a framework just to do res.req = req? ... I don't see the point of this considering how easy this is to do in userland.

The point is not that you need a framework to achieve the linking—I know that it's a one liner.

This issue is talking about the effects of not having that one liner in core at an ecosystem level. Frameworks have proven its utility. And by not having it, it makes it harder for framework-agnostic utility modules to prosper. Which contributes to the ecosystem-wide lock in of HTTP helpers in framework-specific wrappers.

If you're that concerned about the number of parameters (growing or otherwise stable), then use an object for everything else besides req and res.

This doesn't really solve the problem, it just makes it easier to grow over time. All of the confusion/awkwardness around "to req or not" for each helper still stands. And further, it makes these helpers diverge further from simple, functional, method-like utilities.

@ianstormtaylor ianstormtaylor changed the title attach res as req.res attach req as res.req Jul 13, 2019
@boneskull
Copy link
Contributor

I like this idea in theory because of “enabling framework-agnostic modules”.

I think it’d help to have an example of a real-world situation where this would help.

I’d also encourage you to open a PR—you’re likely to get more engagement from collaborators.

@ianstormtaylor
Copy link
Contributor Author

ianstormtaylor commented Jul 14, 2019

I’d also encourage you to open a PR—you’re likely to get more engagement from collaborators.

That's totally fair! I'm more than happy to submit a pull request if the Node team signs off on it. Just don't want to spend time to have it shot down, since I haven't contributed to core I'm unsure how much of a time sink it is.

I think it’d help to have an example of a real-world situation where this would help.

I went into it a bit in the original post, but I can try to point to more...


One example is for a set of HTTP helpers. I've wanted to create:

getHeader(req, name)
getCharset(req)
getType(req)
...

setHeader(res, name, value)
setAttachment(res, filename)
setAuthentication(res, schemes)
setCharset(res, charset)
setType(req, type)
...

This works well for 90% of the functions. All of the get* functions take a signature of (req, ...args) and all of the set* helpers take (res, ...args). This is really intuitive for people.

But there are a few where the set* helpers need access to the req object as well. For example, setCors isn't possible as (res, ...) because it needs to handle OPTIONS requests. There are other edge cases like this that crop up for low-level HTTP compliance reasons, like handling HEAD, or handling Proxy-* headers. Things that the user doesn't really need to care about, but that make it non-compliant.

This is unfortunate, since it ruins the consistency of the API.

(Even further... these helpers translate very nicely to methods, such that you can use the createServer(options, handler) API to pass in your own Request/Response constructors, and things become seamless. This isn't necessary, but it's a nice thing to unlock. But needing req for some of the methods makes it awkward. For example res.setCors(req) is odd and breaks the pattern.)


Another example from in the wild is a "send" utility for sending response bodies simply. If you look at the Micro framework, they actually ship a send(res, status, body) helper. And their helper doesn't currently handle HEAD requests properly because it has no access to the req object.

They'd need to either change the signature helper in a breaking change, in a way that makes the 90% case feel unintuitive. Or they'd need to attach res.req to be able to handle the edge case under the covers. Of course, they can choose to do this, since they are a framework. But trying to implement the same send helper that is HEAD-aware in userland without any framework is currently impossible because you'd have to change the signature.

Edit: HEAD is handled automatically in Node's core by caching state from the req when the res was first created. This reinforces the idea that response-related logic sometimes needs access to the req object for HTTP-specific edge/side cases like these.


If others have example I'd love to hear them. Anything where the 90% case involves just res, but there exist edge cases that require accessing metadata in req will probably have this awkwardness in its API currently.

@ianstormtaylor
Copy link
Contributor Author

Another way to look at what kinds of use cases are hampered by this is to look at Express's existing res.* methods. Since they expose res.req internally, they can make use of it whenever needed. And if you look at their source, all of these methods access res.req:

  • res.send()
  • res.jsonp()
  • res.sendFile()
  • res.format()
  • res.cookie()
  • res.location()
  • res.redirect()
  • res.render()

It gets used for checking things like if the method is HEAD, checking the "freshness" of the request in the cache, checking Accept-* headers for returning the proper content, checking the Referer header for added convenience while redirecting, etc.

There are lots of these little edge cases where you wouldn't realize that you need access to req but you do for completeness.

A framework-agnostic set of helpers would need it slightly less, since things like cookies configuration can be decoupled. But there are still a significant amount of cases that end up using the res.req link under the hood to provide the expected HTTP behaviors.

@dougwilson
Copy link
Member

And their helper doesn't currently handle HEAD requests properly because it has no access to the req object.

In what way does Micro's send not currently handle HEAD requests properly? It looks proper to me; Node.js will actually automatically discard the data sent to res.write/res.end when the request had the HEAD method (https://github.com/nodejs/node/blob/master/lib/_http_server.js#L148 , https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L573).

@ianstormtaylor
Copy link
Contributor Author

It looks proper to me; Node.js will actually automatically discard the data sent to res.write/res.end when the request had the HEAD method

Interesting! That's great to know, I'll strike out that example from above then. I didn't realize that with all the indirection in the core response handling—but that makes sense. Actually, based on the responses to issues/PRs in Micro it sounds like the maintainers of Micro weren't aware of that either. I assume that means that these lines in Express are unnecessary?

Either way though... that further reinforces the idea that req should be exposed, like I mentioned above. When creating the res, the req happens to be in scope to determine the _hasBody property for that one use case. But then userland code can't do the same for the other HTTP-related use cases because req is no longer available as attached to res.

@addaleax addaleax added feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem. labels Jul 14, 2019
XadillaX added a commit to XadillaX/node that referenced this issue May 28, 2020
Add `res.req` and `res.redirect()` in http server for convenience.

Fixes: nodejs#28673
ianstormtaylor added a commit to ianstormtaylor/node that referenced this issue Jan 16, 2021
This change makes it possible for userland http-related modules
to pave over edge cases that require referencing the original request
when handling a response--making a "Lodash for HTTP" library possible.
More information and research in nodejs#28673

Fixes: nodejs#28673
@aduh95 aduh95 closed this as completed in fc3f1c3 Jan 18, 2021
ruyadorno pushed a commit that referenced this issue Jan 22, 2021
This change makes it possible for userland http-related modules
to pave over edge cases that require referencing the original request
when handling a response--making a "Lodash for HTTP" library possible.
More information and research in #28673

Fixes: #28673

PR-URL: #36505
Reviewed-By: Robert Nagy <ronagy@icloud.com>
ruyadorno pushed a commit that referenced this issue Jan 25, 2021
This change makes it possible for userland http-related modules
to pave over edge cases that require referencing the original request
when handling a response--making a "Lodash for HTTP" library possible.
More information and research in #28673

Fixes: #28673

PR-URL: #36505
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants