Skip to content

Support upgrading requests from the adapter#90

Open
Kyiro wants to merge 2 commits intohonojs:mainfrom
Kyiro:upgrade-pr
Open

Support upgrading requests from the adapter#90
Kyiro wants to merge 2 commits intohonojs:mainfrom
Kyiro:upgrade-pr

Conversation

@Kyiro
Copy link

@Kyiro Kyiro commented Nov 8, 2023 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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/undici-types@5.26.5/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/undici-types@5.26.5/node_modules/undici-types/fetch").Request' but 
required in type 'Request'.

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

      node_modules/.pnpm/undici-types@5.26.5/node_modules/undici-types/fetch.d.ts:154:12
        154   readonly duplex: RequestDuplex
                       ~~~~~~
        'duplex' is declared here.
      node_modules/.pnpm/typescript@4.9.5/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/undici-types@5.26.5/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/undici-types@5.26.5/node_modules/undici-types/fetch").Request' but 
required in type 'Request'.

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

      node_modules/.pnpm/undici-types@5.26.5/node_modules/undici-types/fetch.d.ts:154:12
        154   readonly duplex: RequestDuplex
                       ~~~~~~
        'duplex' is declared here.
      node_modules/.pnpm/typescript@4.9.5/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

yusukebe commented Nov 12, 2023

@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

alexfriesen
alexfriesen reviewed Dec 14, 2023
View reviewed changes
src/server.ts

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

@alexfriesen alexfriesen Dec 14, 2023

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 •
edited
Loading

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

@Kyiro Kyiro closed this Jan 18, 2024
@Kyiro Kyiro reopened this Jan 19, 2024
@beeequeue beeequeue mentioned this pull request Jan 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@alexfriesen alexfriesen alexfriesen left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Kyiro @yusukebe @alexfriesen