Skip to content

fix: run onBeforeResponse middleware in declared order#2204

Merged
birkskyum merged 3 commits into
mainfrom
fix/onbeforeresponse-order
Jul 20, 2026
Merged

fix: run onBeforeResponse middleware in declared order#2204
birkskyum merged 3 commits into
mainfrom
fix/onbeforeresponse-order

Conversation

@brenelz

@brenelz brenelz commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Fixes #2131

What is the current behavior?

onBeforeResponse functions passed to createMiddleware execute in reverse array order. h3 v2 middleware compose onion-style, so logic after await next() unwinds innermost-first — and wrapResponseMiddleware does its work after awaiting next(), as noted by @sabercoy in the issue.

What is the new behavior?

The response wrappers are registered reversed, so the onBeforeResponse callbacks run in the order they were declared, matching v1 semantics. onRequest ordering is unchanged.

Other information

Added packages/start/src/middleware/index.spec.ts covering onRequest order, onBeforeResponse order (fails without this fix), request→handler→response sequencing, and response replacement from an onBeforeResponse middleware.

🤖 Generated with Claude Code

h3 v2 middleware compose onion-style, so logic after `await next()`
unwinds innermost-first (reverse registration order). The
onBeforeResponse wrappers do their work after awaiting next(), which
made them execute backwards relative to the array the user passed to
createMiddleware. Register the wrappers reversed so the callbacks run
in their declared order, matching v1 behavior.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Jul 17, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9452baf

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

This PR includes changesets to release 1 package
Name Type
@solidjs/start Patch

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

@netlify

netlify Bot commented Jul 17, 2026

Copy link
Copy Markdown

Deploy Preview for solid-start-landing-page ready!

Name Link
🔨 Latest commit 9452baf
🔍 Latest deploy log https://app.netlify.com/projects/solid-start-landing-page/deploys/6a5e5bb88f239600089e88e9
😎 Deploy Preview https://deploy-preview-2204--solid-start-landing-page.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@pkg-pr-new

pkg-pr-new Bot commented Jul 17, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@solidjs/start@2204

commit: 9452baf

// h3 middleware compose onion-style: logic after `await next()` unwinds
// innermost-first (reverse registration order). Register the wrappers
// reversed so onBeforeResponse functions run in their declared order.
mw.push(...args.onBeforeResponse.map(wrapResponseMiddleware).reverse());

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@katywings you probably know the most about middleware. Does this change make sense?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense yuppa 👍.

What I wonder is why we arent using h3's onRequest and onResponse helpers 🤔. (onRequest does pretty much nothing, but onResponse would simplify our wrapResponseMiddleware)

@brenelz
brenelz requested a review from katywings July 20, 2026 01:26
Comment thread packages/start/src/middleware/index.spec.ts Outdated
@birkskyum

birkskyum commented Jul 20, 2026

Copy link
Copy Markdown
Member

Got some feedback from fable as well:

Suggestions (minor, none blocking)

  • Add a chaining test: nothing currently pins that a later-declared onBeforeResponse sees an earlier one's replacement response as body. That is the other observable behavior the reversal changes (before the fix, the chaining direction was also backwards), and a regression here would be silent.
  • Cheap coverage gaps if you want them: the single-function onBeforeResponse form and the raw Middleware[] passthrough are untested.
  • Release-note consideration: any app that worked around [Bug?]: V2 middleware functions do not execute in order #2131 by pre-reversing its onBeforeResponse array will now get flipped behavior. The changeset describes the fix, which is probably sufficient for a beta line, but it is worth a -sentence in the release notes.
  • Out of scope but worth a follow-up look: because response wrappers are registered after request wrappers, an onRequest that returns a Response short-circuits past all onBeforeResponse callbacks entirely. If v1 ran response hooks even for early-returned responses, that is a separate remaining semantic gap; this PR neither causes nor fixes it.

@brenelz

brenelz commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the feedback

@birkskyum
birkskyum merged commit 1763296 into main Jul 20, 2026
10 checks passed
@birkskyum
birkskyum deleted the fix/onbeforeresponse-order branch July 20, 2026 17:40
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.

[Bug?]: V2 middleware functions do not execute in order

3 participants