Optimisations breaking instanceof Response checks

[ezg27]

Hi there! It looks as though @hono/node-server overriding Web API globals is causing valid instanceof Response checks to fail in Node.js when they succeed in other applications, including the same Hono apps in other runtimes e.g. Bun, Deno, Cloudflare.

It appears to be related to the changes made in the following PR to reduce the overhead of Request and Response instance creation by replacing them with pseudo objects: #95.

Since those changes were released, attempting to do instanceof Response checks against a response returned from native fetch in Node.js will return false (pre v1.3.0 this would correctly return true). Just importing @hono/node-server is enough for the globals to be overridden, so the following is all that's needed to reproduce the issue (using Node v20):

import '@hono/node-server';

const response = await fetch('https://www.example.com');
console.log(response instanceof Response); // false

While i understand the motivation to try and reduce overhead in Node compared to other runtimes, I'm not sure if abstractions like this are the way to go when their implementations can leak into application code. However, i'm also not sure it'll be possible to achieve similar microbenchmark results as those reported in the above PR while still using the the native Request and Response.

For now i can work around the above issues by using patch-package to remove the imports/optimisations from ./request.js and ./response.js so that instanceof checks work as expected. However, if the aim is to keep the current overrides, would you be open to adding an option to disable them as part of the library (e.g. an optional disableOverrides that can be passed to serve)? I'd be happy to raise a PR for this, but will defer to you on the best way forward. At the very least i reckon it's worth documenting the overrides and these potential issues (the README currently still says It utilizes web standard APIs implemented in Node.js version 18 or higher.)

[rafaell-lycan]

I believe this is a great opportunity to also make sure testing regression for cases like this one.

[yusukebe]

Hi @ezg27 !

Sorry for the super delayed reply. This is an important issue, and something must be done about it.

However, if the aim is to keep the current overrides, would you be open to adding an option to disable them as part of the library (e.g. an optional disableOverrides that can be passed to serve)?

This is good. I don't know if the name disableOverrides is appropriate, but we should have an option not to use the Lightweight version of the pseudo object. When it is implemented, it will solve the problems commented here.

@usualoma Do you have any thoughts?

[usualoma]

Hi @ezg27

Thanks for the suggestion!

Yes, I agree. I prefer the option name overrideGlobalObjects and I think something like this would be good.

diff --git a/src/listener.ts b/src/listener.ts
index 0957347..82655d2 100644
--- a/src/listener.ts
+++ b/src/listener.ts
@@ -1,7 +1,7 @@
 import type { IncomingMessage, ServerResponse, OutgoingHttpHeaders } from 'node:http'
 import type { Http2ServerRequest, Http2ServerResponse } from 'node:http2'
-import { getAbortController, newRequest } from './request'
-import { cacheKey, getInternalBody } from './response'
+import { getAbortController, newRequest, Request as LightweightRequest } from './request'
+import { cacheKey, getInternalBody, Response as LightweightResponse } from './response'
 import type { CustomErrorHandler, FetchCallback, HttpBindings } from './types'
 import { writeFromReadableStream, buildOutgoingHttpHeaders } from './utils'
 import { X_ALREADY_SENT } from './utils/response/constants'
@@ -139,8 +139,20 @@ const responseViaResponseObject = async (
 
 export const getRequestListener = (
   fetchCallback: FetchCallback,
-  options: { errorHandler?: CustomErrorHandler } = {}
+  options: {
+    errorHandler?: CustomErrorHandler
+    overrideGlobalObjects?: boolean
+  } = {}
 ) => {
+  if (options.overrideGlobalObjects !== false && global.Request !== LightweightRequest) {
+    Object.defineProperty(global, 'Request', {
+      value: LightweightRequest,
+    })
+    Object.defineProperty(global, 'Response', {
+      value: LightweightResponse,
+    })
+  }
+
   return async (
     incoming: IncomingMessage | Http2ServerRequest,
     outgoing: ServerResponse | Http2ServerResponse
diff --git a/src/request.ts b/src/request.ts
index 9bffea8..49613d8 100644
--- a/src/request.ts
+++ b/src/request.ts
@@ -20,9 +20,6 @@ export class Request extends GlobalRequest {
     super(input, options)
   }
 }
-Object.defineProperty(global, 'Request', {
-  value: Request,
-})
 
 const newRequestFromIncoming = (
   method: string,
diff --git a/src/response.ts b/src/response.ts
index a031464..0750dce 100644
--- a/src/response.ts
+++ b/src/response.ts
@@ -80,9 +80,6 @@ export class Response {
 })
 Object.setPrototypeOf(Response, GlobalResponse)
 Object.setPrototypeOf(Response.prototype, GlobalResponse.prototype)
-Object.defineProperty(global, 'Response', {
-  value: Response,
-})
 
 const stateKey = Reflect.ownKeys(new GlobalResponse()).find(
   (k) => typeof k === 'symbol' && k.toString() === 'Symbol(state)'
diff --git a/src/server.ts b/src/server.ts
index 3ee0d02..d1b3146 100644
--- a/src/server.ts
+++ b/src/server.ts
@@ -5,7 +5,9 @@ import type { Options, ServerType } from './types'
 
 export const createAdaptorServer = (options: Options): ServerType => {
   const fetchCallback = options.fetch
-  const requestListener = getRequestListener(fetchCallback)
+  const requestListener = getRequestListener(fetchCallback, {
+    overrideGlobalObjects: options.overrideGlobalObjects,
+  })
   // ts will complain about createServerHTTP and createServerHTTP2 not being callable, which works just fine
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
   const createServer: any = options.createServer || createServerHTTP
diff --git a/src/types.ts b/src/types.ts
index 3dfb2f4..90ed29b 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -69,6 +69,7 @@ type ServerOptions =
 
 export type Options = {
   fetch: FetchCallback
+  overrideGlobalObjects?: boolean
   port?: number
   hostname?: string
 } & ServerOptions
diff --git a/test/request.test.ts b/test/request.test.ts
index 340851a..f86bc65 100644
--- a/test/request.test.ts
+++ b/test/request.test.ts
@@ -1,5 +1,14 @@
 import type { IncomingMessage } from 'node:http'
-import { newRequest, Request, GlobalRequest, getAbortController } from '../src/request'
+import {
+  newRequest,
+  Request as LightweightRequest,
+  GlobalRequest,
+  getAbortController,
+} from '../src/request'
+
+Object.defineProperty(global, 'Request', {
+  value: LightweightRequest,
+})
 
 describe('Request', () => {
   describe('newRequest', () => {
diff --git a/test/response.test.ts b/test/response.test.ts
index 175a340..41e4a9a 100644
--- a/test/response.test.ts
+++ b/test/response.test.ts
@@ -1,8 +1,12 @@
 import { createServer, type Server } from 'node:http'
 import type { AddressInfo } from 'node:net'
-import { GlobalResponse } from '../src/response'
+import { GlobalResponse, Response as LightweightResponse } from '../src/response'
 
-class NextResponse extends Response {}
+Object.defineProperty(global, 'Response', {
+  value: LightweightResponse,
+})
+
+class NextResponse extends LightweightResponse {}
 
 class UpperCaseStream extends TransformStream {
   constructor() {

@yusukebe

How about this?

[yusukebe]

@usualoma

Great! I've used that patch and confirmed it works well. Plus, the problem will be fixed. @hono/vite-dev-server can run on Bun with the patch and disabling overrideGlobalObjects. Thanks!

Let's go with it. Could you create a PR?

[usualoma]

I'll write a test later; hang on.
#156

[ezg27]

Apologies i've only just had chance to look at this again. Agree overrideGlobalObjects is a more appropriate name, and just tried v1.10.0 and it works great! I'll update now and remove the patches i was using, many thanks both!