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 #36505

Merged
merged 1 commit into from
Jan 18, 2021
Merged

Conversation

ianstormtaylor
Copy link
Contributor

@ianstormtaylor ianstormtaylor commented Dec 14, 2020

This change adds a res.req property which refers to the original IncomingMessage request object.

The reasons for this small change are explained in #28673, but a quick summary:

  • All existing Node HTTP frameworks do this under the covers when wrapping http.createServer.
  • Referencing the original request is required to handle certain HTTP edge cases when writing responses.
  • Node's internals already reference the req in multiple places when determining response-specific behavior.
  • Retrieving the res from the req is already possible, this just adds the reverse.
  • This would give userland the ability to easily handle these valid HTTP-related edge cases.
  • This would allow for intuitive Lodash-like HTTP helper libraries, instead of relying on monolithic Express-like frameworks.

I'd recommend reading #28673 in full, I put a lot of thought/research into the different arguments.

You can imagine a world where you aren't forced to use frameworks for HTTP in Node, and instead we might benefit from a Lodash-like utility library with smaller, functional utilities…

image

…this would be helpful not only to those who don't want to use frameworks, but also those attempting to building framework-independent libraries and utilities, which is currently almost impossible in the Node ecosystem.

Thanks!


Also, to be clear, I'm not wed to the res.req naming scheme if it needs to be changed for any reason. As long as there's a way to access the original request I don't care what the way is.

Checklist
  • 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 Dec 14, 2020
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm -0.5 on this change (thanks for opening it though!) since both (req, res) are available anyway in the callback and you can just do res.req = req if you want to. But I won't block.

@ianstormtaylor
Copy link
Contributor Author

ianstormtaylor commented Dec 14, 2020

Hey @benjamingr thank you for the response! I agree that that seems like a solution at first. But I think there's a deeper problem, which is that the further from the initial createServer call the logic is, the harder it gets to have the user do the linking manually. Let me explain...

Your own utility

Say you're writing an HTTP utility yourself. For this example, let's go with setStatus for simply setting a status code. (This is just one example, but there are lots of utilities that run into this problem for handling HTTP edge cases.) You might write it like this:

import statuses from 'statuses'

function setStatus(res, code) {
  if (res.headersSent) return
  this.res.statusCode = code
  if (statuses.empty[code]) {} // redacted: handle empty statuses…
  if (req.httpVersionMajor < 2) res.statusMessage = statuses[code] // oops!
}

Notice how you end up needing req at the last line for plumbing-related reasons. But req isn't something that's really in the user's mental model for setStatus because they are just updating the response—the need for req is an implementation detail.

But oh well, since you control the code you add the res.req = req yourself and update your own utility, and it works.

A library of utilities

Going further, consider now that you're not writing the utility yourself, but using a Lodash-like package for HTTP instead. This is ideal, because writing these is tedious (and error prone) and it would be nice if this was solved ecosystem-wide. To use it:

import { setStatus } from 'httpdash'

// Somewhere in your server...
setStatus(res, 500)

But you run into the same issue. There are three potential solutions that technically solve the problem, but none is great:

  1. The library could require users to res.req = req in the original http.createServer. This solution is technically possible, but very leaky! It will be hard to remember to do and result in confusion for first-time users, all to handle an edge case.

  2. Or the library could change the signature to setStatus(req, res, code). This also technically works, but now you've introduced a new type of API confusion. Some response-related utilities require just res and some require req, res. To the user there's no real way to tell which, because the req is only ever used for HTTP plumbing, ideally they wouldn't need to care and everything would work.

  3. Or the library could bite the bullet and add (req, res, ...) to all of its functions. This again technically solves the problem, but now the API has been complicated in all cases. You get further away from the niceness that monolithic frameworks are able to offer because even simple things like setHeader(res, key, value) become setHeader(req, res, key, value) for no technical reason.

What this boils down to, is that without the reference from res -> req, it becomes very hard for non-monolithic frameworks to offer the clean, easy to understand APIs that the frameworks can.

But it actually gets worse still…

A library using utilities

Finally consider the case where you aren't the user writing the server, and you aren't the library of HTTP utilities either… you're now a separate library that does some sort of HTTP-related behavior—maybe you're a pretty printing library that works across all HTTP frameworks.

You have access to res from when you were initialized. You go to use the cool httpdash package of utilities, but realize that for plumbing reasons you also need to change your API to have the user pass in req as well. Depending on how or where you are initialized this can be a non-trivial change to make!

Because the link (that all frameworks do by default) isn't present in core, it makes it harder for 3rd-hand libraries in the ecosystem to use 2nd-hand libraries like the hypothetical httpdash, and further cements the monopoly of frameworks like Express. People feel like making libraries framework-agnostic is too much work, so they just tightly couple everything to whatever their personal preference for framework is. And the whole ecosystem suffers.


In summary…

Yes, it is technically possible to solve this by the user themselves writing res.req = req, I agree. But the issue is that the entire downstream ecosystem of utilities then requires the user to do this for any smarter utilities to take advantage of it, which results in a catch-22 where it never happens and we never get nice framework-independent utilities.

The link between res and its original req is inherent to the needs of HTTP. This is evidenced by Node core’s internal logic relying on it, and by all of the server frameworks in Node.js having made this link implicit for this exact reason.

Right now these nice HTTP utilities are only available to frameworks because they take control of the entire server-side stack. To make writing this sort of nice utilities in a framework-independent way Node.js needs to make the link in core.

@Trott
Copy link
Member

Trott commented Dec 18, 2020

@nodejs/http This could use some comments/feedback/review.

@ronag
Copy link
Member

ronag commented Dec 19, 2020

I'm not opposed to this.

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.

Please make sure http2 compat behaves the same.

@ronag
Copy link
Member

ronag commented Dec 27, 2020

@ianstormtaylor are you still interested to continue working on this PR?

@ianstormtaylor
Copy link
Contributor Author

@ronag yes! I should be able to get to checking out the HTTP2 compat shim after the holidays to add the same property to it.

@ianstormtaylor ianstormtaylor force-pushed the add-res-to-req-link branch 4 times, most recently from 2ec05d9 to faceae3 Compare January 16, 2021 03:26
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.

LGTM, @benjamingr are you still opposed?

@benjamingr
Copy link
Member

@ronag I am still -0.5 as I think this conflates things but not -1 since requests and responses are already very intertwined and the use case sounds reasonable.

Not blocking, a week has passed, as far as I'm concerned this can land.

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 17, 2021
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jan 18, 2021
@nodejs-github-bot
Copy link
Collaborator

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

PR-URL: nodejs#36505
Reviewed-By: Robert Nagy <[email protected]>
@aduh95
Copy link
Contributor

aduh95 commented Jan 18, 2021

Landed in fc3f1c3

@aduh95 aduh95 closed this Jan 18, 2021
@aduh95 aduh95 merged commit fc3f1c3 into nodejs:master Jan 18, 2021
@targos targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 19, 2021
ruyadorno pushed a commit that referenced this pull request 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 <[email protected]>
ruyadorno added a commit that referenced this pull request Jan 22, 2021
Notable changes:

* buffer:
  * introduce Blob (James M Snell) [#36811](#36811)
  * add base64url encoding option (Filip Skokan) [#36952](#36952)
* crypto:
  * experimental (Ed/X)25519/(Ed/X)448 support (James M Snell) [#36879](#36879)
* doc:
  * add @iansu to collaborators (Ian Sutherland) [#36951](#36951)
  * add @RaisinTen to collaborators (Darshan Sen) [#36998](#36998)
* fs:
  * allow position parameter to be a BigInt in read and readSync (raisinten) [#36190](#36190)
* http:
  * attach request as res.req (Ian Storm Taylor) [#36505](#36505)
  * expose urlToHttpOptions utility (Yongsheng Zhang) [#35960](#35960)
@ruyadorno ruyadorno mentioned this pull request Jan 22, 2021
ruyadorno added a commit that referenced this pull request Jan 22, 2021
PR-URL: #37020

Notable changes:

* buffer:
  * introduce Blob (James M Snell) [#36811](#36811)
  * add base64url encoding option (Filip Skokan) [#36952](#36952)
* crypto:
  * experimental (Ed/X)25519/(Ed/X)448 support (James M Snell) [#36879](#36879)
* doc:
  * add @iansu to collaborators (Ian Sutherland) [#36951](#36951)
  * add @RaisinTen to collaborators (Darshan Sen) [#36998](#36998)
* fs:
  * allow position parameter to be a BigInt in read and readSync (raisinten) [#36190](#36190)
* http:
  * attach request as res.req (Ian Storm Taylor) [#36505](#36505)
  * expose urlToHttpOptions utility (Yongsheng Zhang) [#35960](#35960)
ruyadorno added a commit that referenced this pull request Jan 22, 2021
PR-URL: #37020

Notable changes:

* buffer:
  * introduce Blob (James M Snell) [#36811](#36811)
  * add base64url encoding option (Filip Skokan) [#36952](#36952)
* crypto:
  * experimental (Ed/X)25519/(Ed/X)448 support (James M Snell) [#36879](#36879)
* doc:
  * add @iansu to collaborators (Ian Sutherland) [#36951](#36951)
  * add @RaisinTen to collaborators (Darshan Sen) [#36998](#36998)
  * add @miladfarca to collaborators (Milad Fa) [#36934](#36934)
* fs:
  * allow position parameter to be a BigInt in read and readSync (raisinten) [#36190](#36190)
* http:
  * attach request as res.req (Ian Storm Taylor) [#36505](#36505)
  * expose urlToHttpOptions utility (Yongsheng Zhang) [#35960](#35960)
ruyadorno pushed a commit that referenced this pull request 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 <[email protected]>
ruyadorno added a commit that referenced this pull request Jan 25, 2021
PR-URL: #37020

Notable changes:

* buffer:
  * introduce Blob (James M Snell) [#36811](#36811)
  * add base64url encoding option (Filip Skokan) [#36952](#36952)
* doc:
  * add @iansu to collaborators (Ian Sutherland) [#36951](#36951)
  * add @RaisinTen to collaborators (Darshan Sen) [#36998](#36998)
  * add @miladfarca to collaborators (Milad Fa) [#36934](#36934)
* fs:
  * allow position parameter to be a BigInt in read and readSync (raisinten) [#36190](#36190)
* http:
  * attach request as res.req (Ian Storm Taylor) [#36505](#36505)
  * expose urlToHttpOptions utility (Yongsheng Zhang) [#35960](#35960)
ruyadorno added a commit that referenced this pull request Jan 25, 2021
PR-URL: #37020

Notable changes:

* buffer:
  * introduce Blob (James M Snell) [#36811](#36811)
  * add base64url encoding option (Filip Skokan) [#36952](#36952)
* doc:
  * add @iansu to collaborators (Ian Sutherland) [#36951](#36951)
  * add @RaisinTen to collaborators (Darshan Sen) [#36998](#36998)
  * add @miladfarca to collaborators (Milad Fa) [#36934](#36934)
* fs:
  * allow position parameter to be a BigInt in read and readSync (raisinten) [#36190](#36190)
* http:
  * attach request as res.req (Ian Storm Taylor) [#36505](#36505)
  * expose urlToHttpOptions utility (Yongsheng Zhang) [#35960](#35960)
ruyadorno added a commit that referenced this pull request Jan 26, 2021
PR-URL: #37020

Notable changes:

* buffer:
  * introduce Blob (James M Snell) [#36811](#36811)
  * add base64url encoding option (Filip Skokan) [#36952](#36952)
* doc:
  * add @iansu to collaborators (Ian Sutherland) [#36951](#36951)
  * add @RaisinTen to collaborators (Darshan Sen) [#36998](#36998)
  * add @miladfarca to collaborators (Milad Fa) [#36934](#36934)
* fs:
  * allow position parameter to be a BigInt in read and readSync (raisinten) [#36190](#36190)
* http:
  * attach request as res.req (Ian Storm Taylor) [#36505](#36505)
  * expose urlToHttpOptions utility (Yongsheng Zhang) [#35960](#35960)
@mmarchini
Copy link
Contributor

I guess we missed it, but this should've been flagged as a semver-major change since it breaks code that was previously attaching req to res (because req is a getter, and JavaScript on strict mode doesn't allow assigning to getters). A simplified example of the issue:

'use strict'

class Foo {
}

const foo = new Foo();
foo.req = {};

class Bar {
  get req() {
    return "bar";
  }
}

const bar = new Bar();
bar.req = {} // code will crash here
$ node example.js
bar.req = {}
        ^

TypeError: Cannot set property req of #<Bar> which has only a getter
    at Object.<anonymous> (/dev/fd/11:16:9)
    at Module._compile (node:internal/modules/cjs/loader:1092:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1121:10)
    at Module.load (node:internal/modules/cjs/loader:972:32)
    at Function.Module._load (node:internal/modules/cjs/loader:813:14)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12)
    at node:internal/main/run_main_module:17:47

An example of this in the wild is broken support for http2 on restify with Node.js v15: https://github.com/restify/node-restify/runs/2081960519?check_suite_focus=true#step:5:242

I don't know what we should do here: reverting is also a breaking change. Maybe we should just add a note on the changelog that this was an accidental breaking change?

@mmarchini
Copy link
Contributor

@aduh95 good call

cjihrig added a commit to cjihrig/node that referenced this pull request Mar 11, 2021
The change in nodejs#36505 broke
userland code that already wrote to res.req. This commit updates
the res.req property in the http2 compat layer to be a normal
property.

Fixes: nodejs#37705
cjihrig added a commit to cjihrig/node that referenced this pull request Mar 13, 2021
The change in nodejs#36505 broke
userland code that already wrote to res.req. This commit updates
the res.req property in the http2 compat layer to be a normal
property.

PR-URL: nodejs#37706
Fixes: nodejs#37705
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Mary Marchini <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 16, 2021
The change in #36505 broke
userland code that already wrote to res.req. This commit updates
the res.req property in the http2 compat layer to be a normal
property.

PR-URL: #37706
Fixes: #37705
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Mary Marchini <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 16, 2021
The change in #36505 broke
userland code that already wrote to res.req. This commit updates
the res.req property in the http2 compat layer to be a normal
property.

PR-URL: #37706
Fixes: #37705
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Mary Marchini <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

8 participants