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

fix: middleware wrapper #1588

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

Conversation

edivados
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • infrastructure changes
  • Other... Please describe:

What is the current behavior?

onRequest

Not awaiting sendWebResponse can cause event.handled to not be true by the time h3 checks it and the next hook/handler will potentially be called.
https://github.com/unjs/h3/blob/6d1e48438bcbbecfaafa4b9fb800bfb1a03ee592/src/event/utils.ts#L76-L83
https://github.com/unjs/h3/blob/6d1e48438bcbbecfaafa4b9fb800bfb1a03ee592/src/utils/response.ts#L531

Reproduction:

Open in StackBlitz

Refresh enough times and you will get one of 3 results.

  • Page instead of file content
  • File content with wrong content-type header
  • Crash ERR_STREAM_WRITE_AFTER_END

onBeforeResponse

Calling sendWebResponse in here does not seem to be supported. There are no checks for event.handled by h3 unlike for onRequest and the next hook will be called.
https://github.com/unjs/h3/blob/6d1e48438bcbbecfaafa4b9fb800bfb1a03ee592/src/event/utils.ts#L84-L91

Reproduction:

Open in StackBlitz

Refresh enough times and you will get one of 3 results.

  • Page with wrong content-type header
  • File content with correct content-type header
  • Crash ERR_STREAM_WRITE_AFTER_END

What is the new behavior?

onRequest

Await sendWebResponse

onBeforeResponse

Assign return value to response.body instead of calling sendWebResponse. With this the response value of handler or previous hook will be replaced.

H3 will call sendWebResponse for Response objects.
https://github.com/unjs/h3/blob/6d1e48438bcbbecfaafa4b9fb800bfb1a03ee592/src/app.ts#L185
https://github.com/unjs/h3/blob/6d1e48438bcbbecfaafa4b9fb800bfb1a03ee592/src/app.ts#L277-L279

Other information

Related issues:
#1523
nksaraf/vinxi#292

Copy link

changeset-bot bot commented Jul 23, 2024

⚠️ No Changeset found

Latest commit: 0676725

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@ryansolid
Copy link
Member

ryansolid commented Jul 23, 2024

I had a suspicion there was was some timing disconnect here but I hadn't gotten into it. Your explanation makes sense to me. Very much appreciate you looking into this.

I guess with this we change it to how I'd expect it to work (and how it is documented). I'm just wondering if we should change onBeforeResponses API or if we should make people set the body themselves. I feel like Nitro designed it that way for a reason.

@katywings
Copy link
Contributor

+1 for making it as close to the Nitro/h3 design as possible. One additional thought that I wanna leave here is about h3 v2: the new way will be to return the value/Response/stream directly instead of using sendXYZ(xyz) in eventHandlers. As it currently stands utilities like sendWebResponse will be deprecated in v2 https://github.com/unjs/h3/blob/cda0bb29b243625516aa57e9497126e8d3594c7f/MIGRATION.md#response-handling

@edivados
Copy link
Contributor Author

edivados commented Jul 24, 2024

I don't have much of an opinion about returning a response. It's an additional option that h3 doesn't have. Maybe convenient to some and makes onRequest and onBeforeResponse a bit similar/consistent.

@katywings Unclear to me if this applies to event handler hooks in v2. They don't seem to have changed much and return value of onRequest isn't handled. Maybe work in progress or I am missing something. https://github.com/unjs/h3/blob/cda0bb29b243625516aa57e9497126e8d3594c7f/src/handler.ts#L81

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