Skip to content

fix(cloudflare-cache-binding): use closure instead of detached this in setItem - #794

Open
momomuchu wants to merge 1 commit into
unjs:mainfrom
momomuchu:fix/cloudflare-cache-binding-setitem-this-789
Open

fix(cloudflare-cache-binding): use closure instead of detached this in setItem#794
momomuchu wants to merge 1 commit into
unjs:mainfrom
momomuchu:fix/cloudflare-cache-binding-setitem-this-789

Conversation

@momomuchu

@momomuchu momomuchu commented Jul 4, 2026

Copy link
Copy Markdown

The cloudflare-cache-binding driver's setItem() throws TypeError: Cannot read properties of undefined (reading 'setItemRaw') when storage setItem is called through the normal bare-function path, because it references this.setItemRaw! and this is detached there.

Every other method in this driver uses the in-file closure convention (getCache()) instead of this. This extracts the write logic into a setItemRaw closure and has both setItem and setItemRaw call it directly, removing the this dependency. Pure extract-method, no behavior change beyond fixing the crash.

Added a test that stubs globalThis.caches with a Map-backed fake (no real Cloudflare needed) and round-trips setItem/getItem. It throws the reported TypeError without the fix and passes with it. Build, typecheck and lint clean. The existing real-Workers driver test is describe.skip'd (no caches API in the platform proxy), which is why this shipped uncaught; the fake-caches test covers this bug class.

Closes #789

Summary by CodeRabbit

  • Bug Fixes

    • Improved cache storage writes so values are saved and retrieved more reliably, including correct TTL handling and optional cache tagging.
    • Fixed an issue where cached data could fail to round-trip correctly in Cloudflare environments.
  • Tests

    • Added regression coverage for Cloudflare cache-backed storage to verify item writes and reads work as expected.

setItem called this.setItemRaw, which throws when the method is
invoked without the driver bound as this (the normal call path from
storage.setItem). Extract the write logic into a local setItemRaw
closure, matching the getCache/r() pattern already used by every
sibling method, and have both setItem and setItemRaw call it directly.

Adds a regression test that stubs globalThis.caches with a Map-backed
fake so the round trip is covered without a real Cloudflare runtime.

Fixes unjs#789
@momomuchu
momomuchu requested a review from pi0 as a code owner July 4, 2026 18:09
@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 01a98a29-4998-4f7d-b5d8-f26956f40838

📥 Commits

Reviewing files that changed from the base of the PR and between e6be613 and 6a8584c.

📒 Files selected for processing (2)
  • src/drivers/cloudflare-cache-binding.ts
  • test/drivers/cloudflare-cache-binding-fake.test.ts

📝 Walkthrough

Walkthrough

Refactors the cloudflare-cache-binding driver to extract cache-write logic into a shared internal setItemRaw helper, fixing a broken this binding when setItem is invoked through storage.setItem. Adds a Vitest test using a Map-backed fake caches API to verify the fix.

Changes

Cloudflare cache binding fix

Layer / File(s) Summary
Extract shared setItemRaw helper
src/drivers/cloudflare-cache-binding.ts
Adds a TransactionOptions import and an internal setItemRaw helper that builds the cache key, TTL, Cache-Control/Cache-Tag headers, and a Response, then writes via cache.put. Both the driver's setItem and setItemRaw methods now delegate to this helper instead of the previously broken this.setItemRaw! call.
Fake caches API regression test
test/drivers/cloudflare-cache-binding-fake.test.ts
Adds a Map-backed fake of the Cloudflare caches API (match, put, delete), installs it on globalThis.caches per test via beforeEach/afterEach, and verifies storage.setItem with a TTL round-trips correctly through storage.getItem.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Storage
  participant Driver as CloudflareCacheBindingDriver
  participant Helper as setItemRaw helper
  participant Cache as Cloudflare Cache API

  Storage->>Driver: setItem(key, value, options)
  Driver->>Helper: setItemRaw(key, value, options)
  Helper->>Helper: compute cache key, TTL, headers
  Helper->>Cache: cache.put(request, response)
  Cache-->>Storage: write complete
Loading

Related issues: #789 (cloudflare-cache-binding setItem fails when called through storage.setItem)

Suggested reviewers: pi0

Poem:

A rabbit hopped through cache and key,
"this.setItemRaw" would fail, you see.
Now one true helper does the write,
Fake caches test it, day and night.
Hop, cache, hop — the bug takes flight! 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the primary fix: removing the detached this dependency in setItem.
Linked Issues check ✅ Passed The PR fixes the reported setItem crash and adds a regression test that verifies storage writes work.
Out of Scope Changes check ✅ Passed The helper refactor and test addition are directly related to the Cloudflare cache-binding bug.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

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.

cloudflare-cache-binding setItem fails when called through storage.setItem

1 participant