diff --git a/.changeset/river-theme-dev-cdn-extensions-collision.md b/.changeset/river-theme-dev-cdn-extensions-collision.md new file mode 100644 index 00000000000..558a9a9727e --- /dev/null +++ b/.changeset/river-theme-dev-cdn-extensions-collision.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme': patch +--- + +Fix `theme dev` serving a same-named theme asset in response to a `/cdn/extensions/...` request. When a theme and an installed app extension shared an asset filename (e.g. `app.js`), the local dev server's theme matcher swallowed the extension URL prefix and returned the theme file. Extension asset requests now fall through to the CDN proxy (or to a locally-developed extension's filesystem) as intended. diff --git a/packages/theme/src/cli/utilities/theme-environment/local-assets.test.ts b/packages/theme/src/cli/utilities/theme-environment/local-assets.test.ts new file mode 100644 index 00000000000..3c459f20cbe --- /dev/null +++ b/packages/theme/src/cli/utilities/theme-environment/local-assets.test.ts @@ -0,0 +1,99 @@ +import {findLocalFile} from './local-assets.js' +import {emptyThemeFileSystem, emptyThemeExtFileSystem} from '../theme-fs-empty.js' +import {describe, expect, test} from 'vitest' +import {createEvent} from 'h3' +import {IncomingMessage, ServerResponse} from 'node:http' +import {Socket} from 'node:net' +import type {DevServerContext} from './types.js' + +function createH3Event(path: string) { + const req = new IncomingMessage(new Socket()) + const res = new ServerResponse(req) + req.method = 'GET' + req.url = path + req.headers = {} + return createEvent(req, res) +} + +function buildCtx({ + themeFiles = [] as [string, unknown][], + extFiles = [] as [string, unknown][], +}): DevServerContext { + const themeFs = emptyThemeFileSystem() + for (const [key, value] of themeFiles) themeFs.files.set(key, value as never) + const extFs = emptyThemeExtFileSystem() + for (const [key, value] of extFiles) extFs.files.set(key, value as never) + return {localThemeFileSystem: themeFs, localThemeExtensionFileSystem: extFs} as unknown as DevServerContext +} + +describe('findLocalFile', () => { + const themeAsset = {checksum: 't', key: 'assets/app.js', value: '// theme app.js'} + const extAsset = {checksum: 'e', key: 'assets/app.js', value: '// extension app.js'} + + test('serves a local theme asset for /cdn//assets/', () => { + const ctx = buildCtx({themeFiles: [['assets/app.js', themeAsset]]}) + const result = findLocalFile(createH3Event('/cdn/shop/t/12/assets/app.js?v=1'), ctx) + + expect(result.fileKey).toBe('assets/app.js') + expect(result.file).toBe(themeAsset) + }) + + test('serves a local theme asset for a bare /assets/ request', () => { + const ctx = buildCtx({themeFiles: [['assets/app.js', themeAsset]]}) + const result = findLocalFile(createH3Event('/assets/app.js'), ctx) + + expect(result.fileKey).toBe('assets/app.js') + expect(result.file).toBe(themeAsset) + }) + + test('serves a local extension asset for /ext/cdn/extensions///assets/', () => { + const ctx = buildCtx({extFiles: [['assets/app.js', extAsset]]}) + const result = findLocalFile( + createH3Event('/ext/cdn/extensions/019e1813-804f-7f97-ad2d-278904fdd92f/my-app/assets/app.js'), + ctx, + ) + + expect(result.fileKey).toBe('assets/app.js') + expect(result.file).toBe(extAsset) + }) + + test('does not serve a theme asset in response to /cdn/extensions///assets/ (regression)', () => { + // A request for an installed-app extension asset must not be answered with a + // same-named theme asset. With no local extension file present the request + // must fall through (fileKey === undefined) so getProxyHandler can forward + // it to cdn.shopify.com. + const ctx = buildCtx({themeFiles: [['assets/app.js', themeAsset]]}) + const result = findLocalFile( + createH3Event('/cdn/extensions/019e1813-804f-7f97-ad2d-278904fdd92f/klaviyo-email-marketing-54/assets/app.js'), + ctx, + ) + + expect(result.fileKey).toBeUndefined() + expect(result.file).toBeUndefined() + }) + + test('prefers the local extension asset for /cdn/extensions/... when one exists locally', () => { + // Both filesystems have assets/app.js. The extension matcher must win + // (and the theme matcher must not match this path at all), so the + // extension content is served instead of the theme content. + const ctx = buildCtx({ + themeFiles: [['assets/app.js', themeAsset]], + extFiles: [['assets/app.js', extAsset]], + }) + const result = findLocalFile( + createH3Event('/cdn/extensions/019e1813-804f-7f97-ad2d-278904fdd92f/my-app/assets/app.js'), + ctx, + ) + + expect(result.fileKey).toBe('assets/app.js') + expect(result.file).toBe(extAsset) + }) + + test('returns no match when neither filesystem has the file', () => { + const ctx = buildCtx({}) + const result = findLocalFile(createH3Event('/cdn/shop/t/12/assets/missing.js'), ctx) + + expect(result.fileKey).toBeUndefined() + expect(result.file).toBeUndefined() + }) +}) diff --git a/packages/theme/src/cli/utilities/theme-environment/local-assets.ts b/packages/theme/src/cli/utilities/theme-environment/local-assets.ts index 4b5314b0b2d..e925fef4a42 100644 --- a/packages/theme/src/cli/utilities/theme-environment/local-assets.ts +++ b/packages/theme/src/cli/utilities/theme-environment/local-assets.ts @@ -66,7 +66,17 @@ export function getAssetsHandler(_theme: Theme, ctx: DevServerContext) { }) } -function findLocalFile(event: H3Event, ctx: DevServerContext) { +// Theme assets are served under either a bare `/assets/...` path or the vanity CDN +// prefix `/cdn/.../assets/...`. The negative lookahead prevents this matcher from +// claiming `/cdn/extensions/...` paths, which belong to theme app extensions. +const THEME_ASSET_PATTERN = /^(?:\/cdn\/(?!extensions\/).*?)?\/assets\/([^?]+)/ + +// Theme app extension assets are served under either `/ext/cdn/extensions/...` +// (locally-developed extensions, rewritten by `injectCdnProxy` via EXTENSION_CDN_PREFIX) +// or `/cdn/extensions/...` (storefront-emitted URLs for installed extensions). +const THEME_EXTENSION_ASSET_PATTERN = /^(?:\/(?:ext\/)?cdn\/extensions\/.*?)?\/assets\/([^?]+)/ + +export function findLocalFile(event: H3Event, ctx: DevServerContext) { const tryGetFile = (pattern: RegExp, fileSystem: VirtualFileSystem) => { const matchedFileName = event.path.match(pattern)?.[1] @@ -81,10 +91,13 @@ function findLocalFile(event: H3Event, ctx: DevServerContext) { } } - // Try to match theme asset files first and fallback to theme extension asset files + // Try to match theme asset files first and fallback to theme extension asset files. + // When neither matches (e.g. `/cdn/extensions///assets/` for an + // installed third-party app), the request falls through to `getProxyHandler` which + // forwards to cdn.shopify.com. return ( - tryGetFile(/^(?:\/cdn\/.*?)?\/assets\/([^?]+)/, ctx.localThemeFileSystem) ?? - tryGetFile(/^(?:\/ext\/cdn\/extensions\/.*?)?\/assets\/([^?]+)/, ctx.localThemeExtensionFileSystem) ?? { + tryGetFile(THEME_ASSET_PATTERN, ctx.localThemeFileSystem) ?? + tryGetFile(THEME_EXTENSION_ASSET_PATTERN, ctx.localThemeExtensionFileSystem) ?? { isUnsynced: false, fileKey: undefined, file: undefined,