RFC: drop mutation of IncomingMessage owned fields, write only to new, augmented fields

[cdaringe]

Problem

express mutates req.url, and saves the un-mutated version onto originalUrl, making URL driven generic middleware hard to design.

Ref: chimurai/http-proxy-middleware#723

Apologies if this topic has already been discussed, I did execute a cursory search and didn't see it, but I miss things time to time.

Context

http.IncomingMessage is the http module's owned memory, and thus could be considered memory not open for writing. The writing to this memory prevents me from fixing problematic typescript issues in http-proxy-middleware. To make http-proxy-middleware generic is to decouple it from all frameworks that it supports--express, connect, raw http, koa, fastify, ...the works!

However, because express mutates req.url, I cannot trust http.IncomingMessage::url, and either have to:

1. burden users with passing to HPM the same prefix they use in app.use(prefix, ...), then additionally track that prefix internally to the HPM lib, or
2. have express conditionals within HPM that sniff specifically for indicators of this framework, and use alternate logic.
   1. this is the current state of http-proxy-middleware, but to make it play nicely with other server providers, we're now trying to avoid this

Solutions

1. write to new, owned memory only:

// GET /api/foo/bar
req.url // untouched, /api/foo/bar

app.use('/api', ...)
req.express.url // /foo/bar
req.express.base // /api

2. don't mutate URL at all. offer functions for users to derive relative path to the base/prefix path

3. offer a .relativePathname, yielding req.url - sum(...nestedPrefixes)

[dougwilson]

Hello, and sorry you are having trouble. This has been discussed prior and it is a very core feature of express and won't be changing as it is a critical feature for how middleware works.

If you are trying to solve a specific problem, we are happy to help. I suggest perhaps opening a new issue describing the exact issue you are trying to solve so we can assist and perhaps help guide on if there is a feature or bug in express from there.

Asking to remove a core feature thay would break the entire ecosystem is unfortunately not the appropriate starting place for such a conversation.

[dougwilson]

I'll also add that the argument to "do not mutate properties that are part of http.IncomingMessage" are likely mute, as this is a hugely breaking change to middlewares out there that will likely need a phased migration, and Express itself will by that time not even be using http.IncomingMesage in order to support http/2, meaning req.url would acrually belong to express proper anyway, and not http.IncomingMesaage :)

[cdaringe]

If you are trying to solve a specific problem, we are happy to help. I suggest perhaps opening a new issue

I am. I'd be happy to add greater details as requested. Please kindly ack that I did describe the problem in the # Problem + # Context sections. A new issue will look much like the current one, so to best serve you, please let me know where you'd like me expand on.

not the appropriate starting place for such a conversation.

Respectfully, why not? I described a problem, then provided ideas & possible fixes to the problem. Your replies do not address the merits of the issue, which is fine. You owe me nothing (a random dude on the internet). I get that. But as a daily participant across OSS projects, GitHub issues are precisely the place for such conversation, in my experience.

Asking to remove a core feature thay would break the entire ecosystem is unfortunately

I understand migrations can be tedious. Nonetheless, SemVer, versioning at large, hedge this. I read this remark as "we can't do breaking changes", and wasn't clear if that's what you really meant, or ...something else perhaps.

to support http/2, meaning req.url would acrually belong to express proper anyway, and not http.IncomingMesaage :)

I'll trust your judgement here, but perhaps you'd be willing to help me understand further? Using the below as context,

const http2 = require("http2");
const fs = require("fs");
const server = http2.createSecureServer({ /* snip */ });
server.on("request", (req) => {
  req.url; // Http2ServerRequest::url
});

I'd assume that a req reference may be still be captured and exposed by express in the future? For instance, ATM express gives us (req, res, next) => .... unsure what the http2 intfc looks like, but is req now hidden?

[dougwilson]

Hi @cdaringe I took a look at your referenced issue, and it seems to me that the issue you are running in to is working against the Express mounting API (app.use), rather than integrating it. For example, a proxy module like app.use('/api', proxy('http://localhost')) would be expected to proxy http://localhost/api/foo to http://localhost/foo in the Express API. It seems that the middleware is re-inventing a lot of the path removal and such Express API already provides, which is what seems to be at odd with the req.url alteration that app.use provides. What is the specific reasoning for doing that? It seems that if the module worked with the Express API rather than against it, there would be no need to ever reference req.originalUrl in that middleware as far as I can tell.

I'd assume that a req reference may be still be captured and exposed by express in the future? For instance, ATM express gives us (req, res, next) => .... unsure what the http2 intfc looks like, but is req now hidden?

That is correct, or perhaps as req.raw like other frameworks, but the actual Node.js request/response will no longer be the object that is actually passed around; at best it is a reference to one.

If you really want to focus on breaking the existing req.url in order to support your use-case, my point is that with the migration for http/2 + beyond uplift would likely already solve your use-case of getting an unmodified original object from the Express API in the same time frame, but would not require breaking the ecosystem of existing middlewares.

[cdaringe]

Hey Doug, great points. I was maintaining existing functionality, but in retro I think the existing functionality is exactly as you describe--counter to express' design, when it doesn't need to be. Will take it up off yonder