Code Monkey home page Code Monkey logo

Comments (6)

usualoma avatar usualoma commented on July 28, 2024 2

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

from node-server.

usualoma avatar usualoma commented on July 28, 2024 1

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?

from node-server.

yusukebe avatar yusukebe commented on July 28, 2024 1

@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?

from node-server.

ezg27 avatar ezg27 commented on July 28, 2024 1

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!

from node-server.

rafaell-lycan avatar rafaell-lycan commented on July 28, 2024

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

from node-server.

yusukebe avatar yusukebe commented on July 28, 2024

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?

from node-server.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.