Skip to content

fix: make RequestContext required on matchHeaders/matchRedirect/matchRewrite#213

Merged
southpolesteve merged 1 commit intomainfrom
fix/require-ctx-in-matchers
Mar 1, 2026
Merged

fix: make RequestContext required on matchHeaders/matchRedirect/matchRewrite#213
southpolesteve merged 1 commit intomainfrom
fix/require-ctx-in-matchers

Conversation

@southpolesteve
Copy link
Copy Markdown
Collaborator

Summary

  • Makes ctx: RequestContext a required parameter on matchHeaders, matchRedirect, and matchRewrite in config-matchers.ts (and their wrapper functions in index.ts)
  • Fixes deploy.ts where matchHeaders was called without reqCtx (the only call site missing it)
  • Updates tests to pass a minimal RequestContext instead of relying on the previously optional parameter

Why

Next.js always has request context when evaluating has/missing conditions. There is no code path where these matchers run without a request. Making ctx required:

  1. Eliminates the ambiguous "what if ctx is missing?" question (previously, conditional rules were silently applied without checking conditions)
  2. Catches missing context at compile time instead of causing silent behavioral bugs at runtime
  3. Matches Next.js semantics, where matchHas() takes req as a required parameter

Extracted from #134 by @erayack.

…Rewrite

Next.js always has request context when evaluating has/missing
conditions on headers, redirects, and rewrites. There is no code path
where these are evaluated without a request.

Making ctx required eliminates ambiguity about what happens when
context is missing, and surfaces any future call sites that forget
to pass it as compile-time errors instead of silent behavioral bugs.

Also fixes deploy.ts matchHeaders call that was missing reqCtx.
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Mar 1, 2026

Open in StackBlitz

npm i https://pkg.pr.new/vinext@213

commit: 30fe6b1

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 1, 2026

Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original

@southpolesteve
Copy link
Copy Markdown
Collaborator Author

/bonk review please

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

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

Clean, well-scoped change. All call sites already had reqCtx available — the only one missing it (deploy.ts line 601) is now fixed. Test updates correctly reflect the new semantics (the backward-compat test is rightly replaced with a test that verifies conditions are actually checked).

Verified:

  • All callers in index.ts (applyHeaders, applyRedirects, applyRewrites) already constructed reqCtx and passed it through — the wrapper signatures just now enforce what was already the practice.
  • All callers in prod-server.ts already pass reqCtx (lines 828, 836, 854, 887, 906) — no changes needed there.
  • All callers in deploy.ts (the generated Pages Router worker entry) already pass reqCtx at lines 609, 623, 643, 660 — the one omission at line 601 (matchHeaders) is the bug this PR fixes.
  • Test coverage is good: the old backward-compat test ("keeps backward-compatible behavior when ctx is omitted") that asserted conditional rules were silently applied is replaced with a test that verifies they are properly skipped when the condition isn't met.

LGTM.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Mar 1, 2026

PR #213 reviewed and approved.

Summary: This is a clean, well-scoped fix. The change makes ctx: RequestContext required on matchHeaders, matchRedirect, and matchRewrite — which matches how Next.js always has request context when evaluating has/missing conditions.

Key findings:

  • All call sites in index.ts, prod-server.ts, and deploy.ts already had reqCtx constructed and were passing it — except for a single bug in deploy.ts:601 where matchHeaders was called without it. That's the concrete bug this PR fixes.
  • The wrapper functions in index.ts (applyHeaders, applyRedirects, applyRewrites) now enforce at the type level what was already the practice at every call site.
  • The test update correctly replaces the old "backward-compatible behavior when ctx is omitted" test (which asserted that conditional rules were silently applied without checking conditions) with a test that verifies conditions are properly evaluated.

github run

@southpolesteve southpolesteve merged commit 512bf0d into main Mar 1, 2026
23 checks passed
@southpolesteve southpolesteve deleted the fix/require-ctx-in-matchers branch March 1, 2026 18:03
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.

2 participants