Skip to content

Conversation

EdamAme-x
Copy link
Contributor

@EdamAme-x EdamAme-x commented Jan 9, 2025

resolve: honojs/hono#3293

Still some concerns and bugs.

The author should do the following, if applicable

  • Add tests
  • Run tests
  • yarn changeset at the top of this repo and push the changeset
  • Follow the contribution guide

Copy link

changeset-bot bot commented Jan 9, 2025

🦋 Changeset detected

Latest commit: b393f28

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@hono/connect Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@EdamAme-x EdamAme-x marked this pull request as draft January 9, 2025 02:11
@EdamAme-x
Copy link
Contributor Author

hi @yusukebe

There are two problems with this implementation.

The first is regarding the header update process.

This test fails.
https://github.com/honojs/middleware/pull/928/files#diff-05c9dabcbf8c76abe4ba387843468024649359e039e0f521ed755f44de39ca44R38-R43

Here is the cause.
https://github.com/honojs/middleware/pull/928/files#diff-e82bf8d64b2d70f2d39b9aa64ada59b6d93549da4aa4f72f7c9f15e3d7426fb0R43

The header is not removed successfully.

@EdamAme-x
Copy link
Contributor Author

EdamAme-x commented Jan 9, 2025

Second, even using “next” internally does not count as Hono's overall middleware.
It is instantiated once internally and the request is passed on.

However, this problem can be sufficiently avoided by being careful about the order of execution.

(It is obvious that the performance is quite bad)

@EdamAme-x EdamAme-x changed the title feat: an middleware for compatibility with express/connect for Hono feat: middleware for compatibility with express/connect for Hono Jan 15, 2025
@EdamAme-x
Copy link
Contributor Author

EdamAme-x commented Jan 15, 2025

ToDo: mark as dev deps
"helmet": "^8.0.0",

Co-Authored-By: sor4chi (no-reply@github.com)
@EdamAme-x EdamAme-x marked this pull request as ready for review January 15, 2025 03:45
@EdamAme-x
Copy link
Contributor Author

EdamAme-x commented Jan 15, 2025

hi @yusukebe @sor4chi
Do you know any reason why the header cannot be removed successfully here?

https://github.com/honojs/middleware/pull/928/files#diff-e82bf8d64b2d70f2d39b9aa64ada59b6d93549da4aa4f72f7c9f15e3d7426fb0R43

@yusukebe
Copy link
Member

Hi @EdamAme-x

I think this approach is okay (actually, there is only this way), though the overheads are heavy as you said. I'll check the problems.

@EdamAme-x EdamAme-x marked this pull request as draft January 15, 2025 10:09
@dajulia3
Copy link

Just here to say that this would be incredibly useful. If you need any additional help or eyes on test edge cases let me know.

@kirill-dev-pro
Copy link

Is there something I could help with this PR? Would be great to have it merged

@EdamAme-x
Copy link
Contributor Author

There are still a few bugs that need to be fixed.

@mbrevda
Copy link

mbrevda commented Apr 26, 2025

There are still a few bugs that need to be fixed.

Can you elaborate? Perhaps someone can jump in here and help

@mridang
Copy link

mridang commented Jul 1, 2025

Hi @EdamAme-x any help needed with the issue? One thing I've found is that there is a chance of of Prematurely Finalized Response being Ignored.

The potentiald bug is in how the middleware handles responses sent directly from a Connect middleware (e.g., res.send(), res.end()).

IIRC, in Hono, when a middleware sets a response directly on the context (c.res = ...), it must also set c.finalized = true which tells the Hono engine that response is complete and to stop processing any further middleware or handlers and send this response immediately.

thhe missing c.finalized in packages/connect/src/index.ts, the code block is if (res) { c.res = res; }, which is missing c.finalized = true;. This bug might prevent a middleware from successfully ending the request chain.

I also noticed that you are overwriting the headers.

const connectResponse = transformResponseToServerResponse(response);
        const preparedHeaders = (c.newResponse(null, 204, {})).headers;
        const connectHeaders = connectResponse.headers;

        for (const key of [...preparedHeaders.keys()]) {
          c.header(key, undefined);
        }

        for (const [key, value] of [...connectHeaders.entries()]) {
          c.header(key, value);
        }

Wouldn't this cause some middleware headers to be lost?

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.

Support for Express.js Compatible Middleware
6 participants