Direct fetch is incompatible with express.js

[Tofandel]

Environment

Nuxt with default node env

Reproduction

import createApp from 'express';
import {IncomingMessage} from 'unenv/runtime/node/http/_request'
import {ServerResponse} from 'unenv/runtime/node/http/_response'

const req = new IncomingMessage();
const res = new ServerResponse(req);

// Taken from express init middleware implementation
const app = createApp();
Object.setPrototypeOf(res, app.response);

res.setHeader('vary', 'test'); // TypeError: Cannot set properties of undefined (setting 'vary')

Describe the bug

When sending a local request to an express server using fetch, a server error occurs

Additional context

This is very problematic in nuxt with useFetch and a legacy endpoint that uses an express server behind the scenes (This issue took me about 20h to debug because it's such a complex chain of events to follow and the errors hidden behind 3 layers could not even be sent properly because the response object was broken)

It seems the problem is that unenv's fetch always uses its internal node/http/request,response even if build with the node env preset and this is problematic because then local requests behave differently than external requests
https://github.com/unjs/unenv/blob/main/src/runtime/fetch/call.ts#L1-L2

unenv's fetch is then used by nitro even if env is set to node
https://github.com/unjs/nitro/blob/a399e189576d4c18d7f837bd454e0503e1f53980/src/types/runtime/nitro.ts#L7

Logs

No response

[Tofandel]

What would be the best way to solve this issue?
From the top of my head I could see a few solutions

1. We could export 2 differents unenv/runtime/fetch/index using the package.json exports field and specify one for node and one for default

2. We could have IncomingMessage and ServerResponse files function as polyfills and if importing node:http fails then we export the unenv implementation, otherwise we export the node implementation

3. We could alias in the node preset unenv/runtime/node/http/_request and response to node:http/request (but a few tweaks to avoid deprecation warnings is necessary)

[pi0]

Using this minimal reproduction to followup:

import express from 'express';
import { createFetch, createCall } from "unenv/runtime/fetch/index"; // [email protected]
const app = express().use('/test', (req, res) => {
  try {
      res.setHeader('vary', 'test');
    res.send('Hello World')
  } catch (err) {
    console.error(err)
    throw err
  }
})
const serverFetch = createFetch(createCall(app));
const res = await serverFetch("/test", {});
console.log({
  headers: res.headers.entries(),
  body: await res.text()
})

TypeError: Cannot convert undefined or null to object
    at ServerResponse.removeHeader (node:_http_outgoing:799:30)

---

Ideally, express should use the runtime IncomingMessage / ServerResponse prototypes (which in case of direct fetch in nitro, is polyfill). (I noticed you made expressjs/express#6039 upstream issue thanks 🙏🏼 )

Alternatively we could make unenv polyfill compat by exposing [kOutHeaders] prop (issue is that Symbol is node internal!)

[Tofandel]

I was thinking that the best way would be that in the node preset, we just alias unenv/runtime/node/http/_request and response to the native node request and make sure the createCall implementation works with the native module

Why use the polyfill when have the natives available?

Ideally, express should use the runtime IncomingMessage / ServerResponse

The problem is they are doing some weird thing instantiating request and response before runtime fetch, so you can add your own methods to them. Their approach is bad practice all over really, they should have just exposed an object and used object assign on req and res to add the methods without messing with prototypes. I opened expressjs/express#6047 which would fix this by stopping the prototype inheritance to only the methods added to the express req and res

But I still think unenv should not polyfill those in the node preset, if I am on node, I expect it to behave using the native and not polyfills

[pi0]

Node.js native IncomingMessage / ServerResponse is not intended for usage of unenv/fetch utility (it is not a polyfill)

We are emulating an HTTP request without actually making a network round trip (otherwise you need to listen an express server and fetch with absolute URL. relative URL fetch is also not supported by node on server alone -- which is your case).

IncomingMessage constructor of node expects an actual Socket and ServerResponse needs request socket info to send it back. (to an actual network socket)

[Tofandel]

it is not a polyfill

Right, it's an emulation taht uses the polyfill, but at the end of the local request, it is expected to be a server running on the env set by the preset, so if it has the node preset, it means the server at the end normally works on the native http implementation, it's expected to have the exact same behavior on the direct call, and having a mix of sometimes the native implementation and sometimes the polyfill in res and req, based on if it's a direct request or not leads to those edge cases

I guess this could be addressed by having a different polyfill for node, where we just override what we need to make it work and alias to that instead in the preset

Something like

export class IncomingMessage extends http.IncomingMessage { // Extend the native implementation
  // Override some stuff
}

I can see a way to avoid the duplicated code by having in the original

export class ServerResponse extends NodeServerResponseOrWritable

Where NodeServerResponseOrWritable will resolve to either Writable or the http.IncomingMessage with a proxy file and aliasing

Otherwise the parameters you mention are not required and const res = new http.IncomingMessage(); const req = new http.Request(res) do work on their own and methods don't seem to error, so with some events on the http.Request we could emulate it properly

node -e "import {IncomingMessage, ServerResponse} from 'node:http';const req = new ServerResponse(new IncomingMessage()); req.write('test'); req.end(); console.log(req.outputData)"

[pi0]

it is expected to be a server running on the env set by the preset

No actually the util does not assumes on a running server that's the whole point/benefit of direct fetch util.

I guess this could be addressed by having a different polyfill for node, where we just override what we need to make it work and alias to that instead in the preset

While i agree that it could be one possible solution to extend nodeHTTP.IncomingMessage I'm inclined to avoid doing this (mixing with built-in) as much as possible as it makes unenv behavior dependent on runtime internals by extending their built-in classes. (if there is matter of performance also BTW it could actually have more overhead than less, Node.js lib is also some js code)

We have Node.js, Bun, Deno, (and workerd maybe someday?) with at least 3 different (internal) implementations of http.IncomingMessage then to care of and I'm afraid it is not just matter of extending.

The root cause of express incompatibility is actually a wrong mix that express tries to call http.IncomingMessage.removeHeader on an incompatible class (and your PR upstream is fixing it), by extending, actually we increase the similar chances.

It probably makes sense for node:stream to be extended from runtime's built-in (#365 > solves #332.) because built-in node streams could be compatible but not for node:http

[pi0]

With all this said, if you still like to invest time on making a node:http mocking solution with extending node built-in IncomingMessage/ServerResponse, I would love to see a draft PR and evaluate, not strictly against it.

But it will be a long path and we need someone to constantly maintain it ensuring will be compatible with future runtime changes.

This time could be spent on better places such as stream support (#365) I can invite you to another effort we are trying to make sure unenv impl is same as Node.js for critical modules as as node:streams)