-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: auto-inject experimental.cache provider so emdash routes do not crash #963
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "emdash": patch | ||
| --- | ||
|
|
||
| Fixes a runtime crash where API routes (publish, unpublish, schedule) and templates (`Astro.cache.set`) hit `Cannot read properties of undefined` when the host project hadn't opted into Astro's `experimental.cache`. The integration now auto-injects `memoryCache()` as the default provider; hosts that already configured a provider keep theirs. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| */ | ||
|
|
||
| import type { AstroIntegration, AstroIntegrationLogger } from "astro"; | ||
| import { memoryCache } from "astro/config"; | ||
|
|
||
| import { validateAllowedOrigins, validateOriginShape } from "../../auth/allowed-origins.js"; | ||
| import type { ResolvedPlugin } from "../../plugins/types.js"; | ||
|
|
@@ -271,11 +272,34 @@ export function emdash(config: EmDashConfig = {}): AstroIntegration { | |
| }, | ||
| ]; | ||
|
|
||
| // EmDash relies on Astro's experimental response cache: | ||
| // - API routes (publish, unpublish, schedule, etc.) call `cache.invalidate()` | ||
| // to bust tagged entries when content changes. | ||
| // - Templates and user pages call `Astro.cache.set(cacheHint)` so list/detail | ||
| // routes are cacheable per-tag. | ||
| // When the host project does not opt into `experimental.cache`, both call sites | ||
| // hit `undefined` at runtime and crash with `Cannot read properties of undefined`. | ||
| // Auto-inject `memoryCache()` as the default provider so the contract holds for | ||
| // every host (graft path, starter templates, Deploy-to-Cloudflare button, etc.). | ||
| // Hosts that already configured a provider keep theirs. | ||
| const existingCacheProvider = ( | ||
| astroConfig.experimental as { cache?: { provider?: unknown } } | undefined | ||
| )?.cache?.provider; | ||
| const cacheConfig = { | ||
| cache: { | ||
| provider: existingCacheProvider ?? memoryCache(), | ||
| }, | ||
| }; | ||
|
|
||
| updateConfig({ | ||
| security: securityConfig, | ||
| // fonts is a valid AstroConfig key but may not be in the | ||
| // type definition for the minimum supported Astro version | ||
| ...({ fonts: emdashFonts } as Record<string, unknown>), | ||
| // 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>), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The cast comment says "experimental typing varies across Astro minor versions". One concrete thing to note for future maintainers: this works because |
||
| vite: createViteConfig( | ||
| { | ||
| serializableConfig, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,79 @@ | ||||||||||||
| import { describe, expect, it, vi } from "vitest"; | ||||||||||||
|
|
||||||||||||
| // `createViteConfig` resolves `@emdash-cms/admin/dist` via require.resolve, which | ||||||||||||
| // fails in a fresh checkout where the admin package hasn't been built yet. Stub | ||||||||||||
| // it: this test only cares about the experimental.cache injection in | ||||||||||||
| // `updateConfig({...})`, not the vite config payload. | ||||||||||||
| vi.mock("../../../../src/astro/integration/vite-config.js", () => ({ | ||||||||||||
| createViteConfig: () => ({}), | ||||||||||||
| })); | ||||||||||||
|
|
||||||||||||
| const { emdash } = await import("../../../../src/astro/integration/index.js"); | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * Regression tests for the experimental.cache provider auto-injection. | ||||||||||||
| * | ||||||||||||
| * Without this, hosts that haven't opted into `experimental.cache` get an | ||||||||||||
| * undefined `cache` parameter in the API route context, and any call to | ||||||||||||
| * `cache.enabled` / `cache.invalidate()` (publish, unpublish, schedule, etc.) | ||||||||||||
| * or `Astro.cache.set(cacheHint)` (templates) crashes with | ||||||||||||
| * `TypeError: Cannot read properties of undefined`. | ||||||||||||
| * | ||||||||||||
| * Tracked in: | ||||||||||||
| * - https://github.com/emdash-cms/emdash/issues/962 (DX umbrella) | ||||||||||||
| * - https://github.com/emdash-cms/emdash/issues/959 (Deploy-to-Cloudflare 500) | ||||||||||||
| * - https://github.com/emdash-cms/emdash/issues/945 (Astro.cache.set undefined) | ||||||||||||
| */ | ||||||||||||
| describe("emdash integration: experimental.cache injection", () => { | ||||||||||||
| function runConfigSetup(hostConfig: Record<string, unknown>) { | ||||||||||||
| const integration = emdash({}); | ||||||||||||
| const setup = integration.hooks["astro:config:setup"]; | ||||||||||||
| if (!setup) throw new Error("astro:config:setup hook missing"); | ||||||||||||
|
|
||||||||||||
| const updates: Array<Record<string, unknown>> = []; | ||||||||||||
| void setup({ | ||||||||||||
| injectRoute: vi.fn(), | ||||||||||||
| injectScript: vi.fn(), | ||||||||||||
| addMiddleware: vi.fn(), | ||||||||||||
| addClientDirective: vi.fn(), | ||||||||||||
| addDevToolbarApp: vi.fn(), | ||||||||||||
| addRenderer: vi.fn(), | ||||||||||||
| addWatchFile: vi.fn(), | ||||||||||||
| updateConfig: (cfg: Record<string, unknown>) => { | ||||||||||||
| updates.push(cfg); | ||||||||||||
| return cfg; | ||||||||||||
| }, | ||||||||||||
| isRestart: false, | ||||||||||||
| logger: { | ||||||||||||
| info: vi.fn(), | ||||||||||||
| warn: vi.fn(), | ||||||||||||
| error: vi.fn(), | ||||||||||||
| debug: vi.fn(), | ||||||||||||
| fork: vi.fn(), | ||||||||||||
| } as never, | ||||||||||||
| config: hostConfig as never, | ||||||||||||
| command: "build", | ||||||||||||
| } as never); | ||||||||||||
|
|
||||||||||||
| return updates; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| it("auto-injects a default cache provider when the host has not opted in", () => { | ||||||||||||
| const updates = runConfigSetup({}); | ||||||||||||
| const cacheUpdate = updates.find((u) => "experimental" in u); | ||||||||||||
| expect(cacheUpdate, "expected an updateConfig call with experimental.cache").toBeDefined(); | ||||||||||||
| const experimental = (cacheUpdate as { experimental: { cache: { provider: unknown } } }) | ||||||||||||
| .experimental; | ||||||||||||
| expect(experimental.cache.provider).toBeDefined(); | ||||||||||||
| expect(experimental.cache.provider).not.toBeNull(); | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assertion is a bit weak —
Suggested change
This pins it to the actual |
||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it("preserves the host's existing cache provider when one is configured", () => { | ||||||||||||
| const hostProvider = { kind: "host-supplied" }; | ||||||||||||
| const updates = runConfigSetup({ experimental: { cache: { provider: hostProvider } } }); | ||||||||||||
| const cacheUpdate = updates.find((u) => "experimental" in u); | ||||||||||||
| const experimental = (cacheUpdate as { experimental: { cache: { provider: unknown } } }) | ||||||||||||
| .experimental; | ||||||||||||
| expect(experimental.cache.provider).toBe(hostProvider); | ||||||||||||
| }); | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider one more test for the case the PR comment specifically mentions: a host that has set other 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. |
||||||||||||
| }); | ||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edge case worth thinking about: the
??falls back tomemoryCache()for any nullish provider, but it also silently overrides cases where a host has setexperimental.cache.providerto something falsy on purpose (e.g. they read a stale tutorial that suggestedprovider: falseto disable, or they spread an env-driven config that leftproviderundefined while keeping othercache.*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.