| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
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 utilitySay 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 utilitiesGoing 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:
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 utilitiesFinally 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. |
Sorry, something went wrong.
|
@nodejs/http This could use some comments/feedback/review. |
Sorry, something went wrong.
|
I'm not opposed to this. |
Sorry, something went wrong.
There was a problem hiding this comment.
Please make sure http2 compat behaves the same.
Sorry, something went wrong.
|
@ianstormtaylor are you still interested to continue working on this PR? |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM, @benjamingr are you still opposed?
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
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 <ronagy@icloud.com>
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>
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)
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>
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)
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)
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)
|
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:47An 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? |
Sorry, something went wrong.
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 <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Mary Marchini <oss@mmarchini.me> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Michaël Zasso <targos@protonmail.com>
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 <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Mary Marchini <oss@mmarchini.me> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Michaël Zasso <targos@protonmail.com>
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 <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Mary Marchini <oss@mmarchini.me> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Michaël Zasso <targos@protonmail.com>
| Back | FazBrowse Home | New Git URL |
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:
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…
…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