Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support upgrading requests from the adapter #90

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Kyiro
Copy link

@Kyiro Kyiro commented Nov 8, 2023

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

@yusukebe
Copy link
Member

yusukebe commented Nov 8, 2023

Hi @Kyiro

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

@yusukebe
Copy link
Member

yusukebe commented Nov 11, 2023

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}`);
})

@Kyiro
Copy link
Author

Kyiro commented Nov 11, 2023

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

@yusukebe
Copy link
Member

@Kyiro,

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

@Kyiro
Copy link
Author

Kyiro commented Nov 12, 2023

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


export const createAdaptorServer = (options: Options): ServerType => {
const fetchCallback = options.fetch
const upgrade = options.upgrade !== false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would always add the upgrade listener unless it is actively set to false. Is this intended?

Copy link
Author

@Kyiro Kyiro Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, idk if it should be the default or not so i made it the default

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants