fix: auto-inject experimental.cache provider so emdash routes do not crash#963
fix: auto-inject experimental.cache provider so emdash routes do not crash#963royachiron wants to merge 2 commits into
Conversation
…rash
Multiple call sites in emdash assume Astro's experimental response cache is
configured:
- API routes (publish, unpublish, schedule, restore, discard-draft, duplicate,
permanent, [collection]/[id], [collection]/index) call cache.invalidate({...})
with a guard on cache.enabled.
- Templates and the docs' "Always call Astro.cache.set(cacheHint)" pattern
rely on Astro.cache.set(...) being a callable method.
experimental.cache is opt-in in Astro 6 and undefined by default. Hosts that
don't enable it (graft path, current starter templates, the Deploy-to-Cloudflare
button output) hit a runtime TypeError on the first publish or homepage render:
TypeError: Cannot read properties of undefined (reading 'enabled')
TypeError: Cannot read properties of undefined (reading 'set')
This patch makes the emdash integration auto-inject memoryCache() as the default
provider. Hosts that already configured a provider keep theirs.
Closes emdash-cms#959, emdash-cms#945. Addresses point 1 of emdash-cms#962.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 6d67fae The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
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 |
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
Putting it out there as clearly as possible - I vibe code simple things. My site, it helps me with QA work, helps with w/e. It's useful. I decided to add a cool blog to my already nifty site. My friend, the code master, tells me of emdash. I like the idea - I don't really dig WP bulkiness. So I ask Claude to add it to my Astro 4 website. It goes through some process, and I end up with a 404, and can't publish anything. He managed to solve it - but I figure - it's a very capable tool - and this issue might have project importance. Documentation? I even asked - But I figure it would be quicker and easier to just act - report the issues - and since I figure it out - I politely asked the AI if it might be able to actually submit a PR. As far as I know - his fix worked on my production environment. Is it the most proper fix? IDK. Is it worth as a PR? No clue, most of it is out of the scope of my capabilities. But I like this project - and I always dreamed of helping, really helping, some OSS. Hope this is a proper PR, and that my contribution is valuable - and not just more slop. |
|
/review |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nice, focused fix. The diagnosis is right: every API route does if (cache.enabled) await cache.invalidate(...), and cache is undefined on hosts that haven't opted into experimental.cache.provider, so even the .enabled access throws. Auto-injecting memoryCache() as a default fixes it without breaking hosts that already configured a provider — Astro's updateConfig uses a recursive mergeConfig, so spreading { experimental: { cache: { provider } } } here deep-merges and preserves any other experimental.* flags the host set (verified in astro/dist/core/config/merge.js).
A few small things below — none are blocking. The first test could be a little stricter, and it's worth a thought about the few edge cases where a host might have explicitly opted out.
| )?.cache?.provider; | ||
| const cacheConfig = { | ||
| cache: { | ||
| provider: existingCacheProvider ?? memoryCache(), |
There was a problem hiding this comment.
Edge case worth thinking about: the ?? falls back to memoryCache() for any nullish provider, but it also silently overrides cases where a host has set experimental.cache.provider to something falsy on purpose (e.g. they read a stale tutorial that suggested provider: false to disable, or they spread an env-driven config that left provider undefined while keeping other cache.* options).
Not a real issue today — Astro's typing rejects boolean providers — but if you wanted to be slightly more conservative, you could check "provider" in (cache ?? {}) instead of relying on ??. Up to you.
| // experimental.cache is opt-in in Astro 6 and undefined by default; | ||
| // cast through unknown because the experimental typing varies across | ||
| // Astro minor versions. | ||
| ...({ experimental: cacheConfig } as Record<string, unknown>), |
There was a problem hiding this comment.
The cast comment says "experimental typing varies across Astro minor versions". One concrete thing to note for future maintainers: this works because updateConfig calls Astro's mergeConfig (in astro/dist/core/config/merge.js), which recurses on nested objects. So pushing { experimental: { cache: { provider } } } here deep-merges with any host-set experimental.session, experimental.routeRules, etc. rather than clobbering them. Worth one sentence in the comment so a future contributor doesn't "simplify" this to a shallow assignment.
| const experimental = (cacheUpdate as { experimental: { cache: { provider: unknown } } }) | ||
| .experimental; | ||
| expect(experimental.cache.provider).toBeDefined(); | ||
| expect(experimental.cache.provider).not.toBeNull(); |
There was a problem hiding this comment.
This assertion is a bit weak — provider will be defined and non-null in any branch (the host case puts the host's provider there, the no-op case puts memoryCache() there). To actually verify the auto-inject branch, check the shape of the returned descriptor:
| expect(experimental.cache.provider).not.toBeNull(); | |
| expect(experimental.cache.provider).toMatchObject({ | |
| name: "memory", | |
| entrypoint: "astro/cache/memory", | |
| }); |
This pins it to the actual memoryCache() descriptor ({ name: "memory", entrypoint: "astro/cache/memory", config: {} }) and would catch a regression where someone accidentally swaps in a different default.
| const experimental = (cacheUpdate as { experimental: { cache: { provider: unknown } } }) | ||
| .experimental; | ||
| expect(experimental.cache.provider).toBe(hostProvider); | ||
| }); |
There was a problem hiding this comment.
Consider one more test for the case the PR comment specifically mentions: a host that has set other experimental.* flags but not experimental.cache. The current code relies on Astro's mergeConfig doing a deep merge on the experimental key — a future refactor that turned the spread into something shallower would silently drop the host's experimental.session/experimental.routeRules/etc. without breaking these tests.
A test like:
it("does not clobber other experimental flags the host set", () => {
const hostExperimental = { routeRules: { "/": { maxAge: 60 } } };
const updates = runConfigSetup({ experimental: hostExperimental });
const cacheUpdate = updates.find((u) => "experimental" in u);
const experimental = (cacheUpdate as { experimental: Record<string, unknown> }).experimental;
// We only assert what this PR sends; merging is Astro's job.
expect(experimental).toHaveProperty("cache.provider");
});would lock the contract on the emdash side.
|
Review posted successfully. The four inline comments cover:
Overall verdict: solid focused bug fix, good comments, reasonable tests — non-blocking suggestions only. |
|
Hey @royachiron – first off, thank you for this. Showing up with a working repro, a changeset, tests, and a complete PR template on your first PR is more than a lot of seasoned contributors manage, and the fact that you described your process honestly in the comment above is exactly the right way to do open source. Don't read what follows as "this is slop" — it isn't. I am going to close this one, but I want to walk through the reasoning carefully because the bug you spotted is real, and the next step is small. The crash stack traces in #945 / #959 ( The workaround for now is a single character: change The reasob auto-injecting There's a Cloudflare-aware provider in The In the meantime, thanks for flagging this. I will land a fix for the immediate bug, and you can update your own site in the same way. |
|
Thank you very much for the honest and detailed review. You could've just wrote something short that details how it's not perfect - instead you delivered in depth analysis of the root cause, so I can understand it even better and learn from the mistakes. |
2 participants
What does this PR do?
Auto-injects a default
experimental.cache.provider(Astro's built-inmemoryCache()) inside the emdash integration so that the route handlers and templates that emdash itself ships do not crash on hosts that haven't opted into Astro 6's experimental response cache.emdash code assumes the cache exists in two places:
publish,unpublish,schedule,restore,discard-draft,duplicate,permanent,[collection]/[id],[collection]/index) callcache.invalidate({ tags: [...] })after writes, guarded only bycache.enabled.Astro.cache.set(cacheHint)on every content page.Both depend on the route context having a
cacheobject. In Astro 6,experimental.cacheis opt-in andundefinedby default. Hosts that don't enable it hit:This is the silent failure paths that bit users in #959, #945, and the publish-flow segment of #962.
The fix is one focused change in
packages/core/src/astro/integration/index.ts: extend the existingupdateConfig({...})call so emdash insertsexperimental.cache.provider: memoryCache()whenever the host config doesn't already have a provider. A host that has configured one keeps theirs. Two new unit tests cover both branches.Closes #959
Closes #945
Addresses point 1 of #962
Type of change
Checklist
pnpm typecheckpasses (no new errors in touched files; pre-existing 8445 errors on main are unaffected)pnpm lintpasses (no new lint errors in touched files; pre-existing 30 errors on main are unaffected)pnpm testpasses (the 2 new tests incache-config.test.tspass; the 9 pre-existing test failures on main are unrelated and unchanged)pnpm formathas been runemdashpatch)AI-generated code disclosure
Screenshots / test output
The two new tests in
cache-config.test.ts:auto-injects a default cache provider when the host has not opted in— asserts that calling the emdash integration'sastro:config:setuphook with an empty host config produces anupdateConfigcall containingexperimental.cache.provider.preserves the host's existing cache provider when one is configured— asserts that a host-supplied provider is passed through unchanged.