Conversation
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds a cache export to cloudflare:workers that proxies through to the current request's CacheContext (i.e. ctx.cache), allowing module-level imports instead of threading the execution context.
Issues (ranked by severity)
-
Missing TypeScript type definitions for the new
cacheexport — The runtime implementation exportscachefromcloudflare:workers, buttypes/defines/rpc.d.ts(which defines theCloudflareWorkersModulenamespace /cloudflare:workersmodule declaration) does not include acacheexport. Users doingimport { cache } from 'cloudflare:workers'will get a TypeScript compilation error even though it works at runtime. You need to add something likeexport const cache: CacheContext | undefined;to theCloudflareWorkersModulenamespace intypes/defines/rpc.d.ts(theCacheContextinterface is already generated from C++ RTTI as a global ambient type) and then runjust generate-typesto update the snapshot. -
Silent
undefinedreturn when accessed outside a request context — WhengetCurrentCache()is called outside an activeIoContext(e.g., at module top-level), every property access on thecacheproxy silently returnsundefined. Compare withwaitUntil, which throws when called outside a request. Silently returningundefinedmakes misuse harder to diagnose. Consider throwing an informative error instead, or document the intended behavior. -
Proxy
gettrap callsgetCurrentCache()on every property access — Each call tocache.purge(...)callsgetCurrentCache()once inget(to look up the method), and operations likeJSON.stringify(cache)orObject.keys(cache)call it multiple times across different traps. This is consistent with how theenvproxy works, so it may be intentional, but worth noting.
|
Review posted successfully on PR #6588. Here's a summary of my findings:
|
348ea17 to
1a74a6d
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
0f82ae9 to
7054827
Compare
bdc12b4 to
c13415b
Compare
c13415b to
6c9d682
Compare
|
The generated output of |
Allow `cache` to be imported from `cloudflare:workers`.
6c9d682 to
61977ab
Compare
Allow
cacheto be imported fromcloudflare:workers.