Skip to content

fix(http): merge default options when partial opts are provided - #2

Merged
pi0 merged 2 commits into
unjs:mainfrom
oritwoen:fix/http-defaults-merge
Mar 19, 2026
Merged

fix(http): merge default options when partial opts are provided#2
pi0 merged 2 commits into
unjs:mainfrom
oritwoen:fix/http-defaults-merge

Conversation

@oritwoen

@oritwoen oritwoen commented Mar 10, 2026

Copy link
Copy Markdown
Contributor

defineCachedHandler used a default parameter value instead of merging defaults into the function body. This meant passing partial options silently dropped all unspecified defaults.

defineCachedFunction already does this correctly (cache.ts line 28):

opts = { ...defaultCacheOptions(), ...opts };

But defineCachedHandler relied on the parameter default, which only applies when no argument is passed at all:

opts: CachedEventHandlerOptions<E> = defaultCacheOptions() // only when opts is undefined

The practical effect: defineCachedHandler(handler, { maxAge: 60 }) left swr as undefined, so the cache-control header logic fell into the non-SWR branch and produced max-age=60 instead of s-maxage=60, stale-while-revalidate. The cache layer itself worked fine (because cachedFunction merges defaults internally), but the HTTP-specific header logic read from the un-merged opts.

I noticed this while reading through the codebase - the inconsistency between how the two functions handle defaults stood out. PLAN.md already had it listed as issue #4.

The fix applies the same spread pattern from defineCachedFunction, plus a regression test that confirms SWR headers are produced when only maxAge is passed.

Summary by CodeRabbit

  • Bug Fixes

    • Ensure default caching options are always merged with user-provided settings so partial configs produce correct cache behavior.
  • Tests

    • Added tests confirming defaults are merged when partial caching options are supplied.
  • Documentation

    • Updated the displayed API signature for the cached handler to reflect the new options behavior.

defineCachedHandler used a default parameter instead of spreading
defaults into the function body. Passing partial options like
{ maxAge: 60 } left swr as undefined, silently producing max-age
instead of s-maxage + stale-while-revalidate headers.

Matches the existing pattern in defineCachedFunction (cache.ts:28).
@coderabbitai

coderabbitai Bot commented Mar 10, 2026

Copy link
Copy Markdown

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e869878e-0993-417a-a11b-88f83405db8a

📥 Commits

Reviewing files that changed from the base of the PR and between 4888786 and 7e5e534.

📒 Files selected for processing (1)
  • README.md

📝 Walkthrough

Walkthrough

Refactored defineCachedHandler to apply default cache options by merging defaults with provided opts inside the function body; added tests verifying partial opts are merged; updated README snippet of the function signature.

Changes

Cohort / File(s) Summary
Default Options Merging
src/http.ts
Changed defineCachedHandler parameter default from defaultCacheOptions() as CachedEventHandlerOptions<E> to {} and now unconditionally merges defaults via opts = { ...defaultCacheOptions(), ...opts } at function start.
Test Coverage
test/index.test.ts
Added two tests ensuring partial options (e.g., maxAge) are merged with defaults and produce expected cache-control headers.
Docs Snippet
README.md
Updated the documented TypeScript snippet for defineCachedHandler, removing the explicit default initializer and the trailing return-type/closure in the snippet.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 I hopped through code with gentle flair,
I spread the defaults everywhere,
No casts, just merges — tidy, neat,
Tests applaud my little feat,
A caching carrot, safe to share.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: fixing how default options are merged when partial options are provided to defineCachedHandler.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@pi0 pi0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thnx!

@pi0
pi0 merged commit 5a71511 into unjs:main Mar 19, 2026
1 check was pending
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