node-server icon indicating copy to clipboard operation
node-server copied to clipboard

Support upgrading requests from the adapter

Open Kyiro opened this issue 2 years ago • 5 comments

These changes allow the upgrading to something like WebSockets from within Hono itself. Feel free to request any changes or even deny this PR as I'm not sure if it fits. The only big issue is that I don't think it supports HTTP2 at all.

class UpgradeResponse extends Response {
    status = 101;
    
    constructor(body?: BodyInit | null, init?: ResponseInit) {
        super(body, init);
    }
}

const app = new Hono<{
    Bindings: {
        incoming: IncomingMessage;
        socket: Socket;
        head: Buffer;
    }
}>();
const wsServer = new WebSocketServer({ noServer: true });

app.get("/", (c) => {
    wsServer.handleUpgrade(c.env.incoming, c.env.socket, c.env.head, (ws) => {
        ws.send("Hello World!");
    });
    
    return new UpgradeResponse();
});

serve(app);

This is only supposed to be an example of what a middleware should do as it's not too clean

Kyiro avatar Nov 08 '23 17:11 Kyiro

Hi @Kyiro

Thanks for the PR. I'll take a look later!

yusukebe avatar Nov 08 '23 23:11 yusukebe

Hi @Kyiro,

This looks good. Using env to handle incoming, socket, and head is a good approach. I have two things for you.

First, could you write proper tests for this implementation?

Second, while it's not necessary to include in this PR, I think passing IncomingMessage to env, even when it's not upgraded, would be a great idea. What are your thoughts? This is related to https://github.com/orgs/honojs/discussions/1439.

app.get('/', (c) => {
  const ip = c.env.incoming.socket.remoteAddress;
  return c.text(`You are accessing from ${ip}`);
})

yusukebe avatar Nov 11 '23 00:11 yusukebe

I wanted to write tests but the types fail for some reason.

 FAIL  test/server.test.ts      
  ● Test suite failed to run

    test/server.test.ts:384:5 - error TS2578: Unused '@ts-expect-error' directive.

    384     // @ts-expect-error: @types/supertest is not updated yet
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

 FAIL  test/serve-static.test.ts
  ● Test suite failed to run

    src/globals.ts:17:19 - error TS2769: No overload matches this call.
      Overload 1 of 2, '(input: RequestInfo, init?: RequestInit | undefined): Promise<Response>', gave the following error.
        Argument of type 'string | Request | URL | URL | Request' is not assignable to parameter of type 'RequestInfo'.
          Type 'Request' is not assignable to type 'RequestInfo'.
            Property 'duplex' is missing in type 'Request' but required in type 'import("C:/Projects/node-server/node_modules/.pnpm/[email protected]/node_modules/undici-types/fetch").Request'.
      Overload 2 of 2, '(input: RequestInfo | URL, init?: RequestInit | undefined): Promise<Response>', gave the following error.
        Argument of type 'string | Request | URL | URL | Request' is not assignable to parameter of type 'RequestInfo | URL'.
          Type 'Request' is not assignable to type 'RequestInfo | URL'.
            Property 'referrer' is missing in type 'import("C:/Projects/node-server/node_modules/.pnpm/[email protected]/node_modules/undici-types/fetch").Request' but 
required in type 'Request'.

    17   return webFetch(info, init)
                         ~~~~

      node_modules/.pnpm/[email protected]/node_modules/undici-types/fetch.d.ts:154:12
        154   readonly duplex: RequestDuplex
                       ~~~~~~
        'duplex' is declared here.
      node_modules/.pnpm/[email protected]/node_modules/typescript/lib/lib.dom.d.ts:11721:14
        11721     readonly referrer: string;
                           ~~~~~~~~
        'referrer' is declared here.

 FAIL  test/vercel.test.ts
  ● Test suite failed to run

    src/globals.ts:17:19 - error TS2769: No overload matches this call.
      Overload 1 of 2, '(input: RequestInfo, init?: RequestInit | undefined): Promise<Response>', gave the following error.
        Argument of type 'string | Request | URL | URL | Request' is not assignable to parameter of type 'RequestInfo'.
          Type 'Request' is not assignable to type 'RequestInfo'.
            Property 'duplex' is missing in type 'Request' but required in type 'import("C:/Projects/node-server/node_modules/.pnpm/[email protected]/node_modules/undici-types/fetch").Request'.
      Overload 2 of 2, '(input: RequestInfo | URL, init?: RequestInit | undefined): Promise<Response>', gave the following error.
        Argument of type 'string | Request | URL | URL | Request' is not assignable to parameter of type 'RequestInfo | URL'.
          Type 'Request' is not assignable to type 'RequestInfo | URL'.
            Property 'referrer' is missing in type 'import("C:/Projects/node-server/node_modules/.pnpm/[email protected]/node_modules/undici-types/fetch").Request' but 
required in type 'Request'.

    17   return webFetch(info, init)
                         ~~~~

      node_modules/.pnpm/[email protected]/node_modules/undici-types/fetch.d.ts:154:12
        154   readonly duplex: RequestDuplex
                       ~~~~~~
        'duplex' is declared here.
      node_modules/.pnpm/[email protected]/node_modules/typescript/lib/lib.dom.d.ts:11721:14
        11721     readonly referrer: string;
                           ~~~~~~~~
        'referrer' is declared here.

Test Suites: 3 failed, 3 total
Tests:       0 total
Snapshots:   0 total
Time:        2.728 s
Ran all test suites.
 ELIFECYCLE  Test failed. See above for more details.
PS C:\Projects\node-server> node -v    
v20.7.0

Kyiro avatar Nov 11 '23 13:11 Kyiro

@Kyiro,

Ah, there's a type mismatch. But could you still push the tests, even if they fail?

yusukebe avatar Nov 12 '23 01:11 yusukebe

Yeah, I guess but I don't have a way to test the tests. I'll see later

Kyiro avatar Nov 12 '23 01:11 Kyiro