From 441dc336de2bc6b62dcda559bcad27e06b88620a Mon Sep 17 00:00:00 2001 From: Fran Dios Date: Fri, 20 Sep 2024 15:51:22 +0900 Subject: [PATCH 1/7] Fix regexp for JS expressions --- packages/theme/src/cli/utilities/theme-environment/proxy.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/theme/src/cli/utilities/theme-environment/proxy.ts b/packages/theme/src/cli/utilities/theme-environment/proxy.ts index 619411574f..bb1d9e2238 100644 --- a/packages/theme/src/cli/utilities/theme-environment/proxy.ts +++ b/packages/theme/src/cli/utilities/theme-environment/proxy.ts @@ -98,8 +98,8 @@ export function injectCdnProxy(originalContent: string, ctx: DevServerContext) { content = content.replace(vanityCdnRE, VANITY_CDN_PREFIX) // -- Only redirect usages of the main CDN for known local theme and theme extension assets to the local server: - const mainCdnRE = /(?:https?:)?\/\/cdn\.shopify\.com\/(.*?\/(assets\/[^?">]+)(?:\?|"|>|$))/g - const filterAssets = (key: string) => key.startsWith('assets') + const mainCdnRE = /(?:https?:)?\/\/cdn\.shopify\.com\/(.*?\/(assets\/[^?#"'`>]+))/g + const filterAssets = (key: string) => key.startsWith('assets/') const existingAssets = new Set([...ctx.localThemeFileSystem.files.keys()].filter(filterAssets)) const existingExtAssets = new Set([...ctx.localThemeExtensionFileSystem.files.keys()].filter(filterAssets)) From 26c13fbca052c4f9731f28200e513afbe4698b9c Mon Sep 17 00:00:00 2001 From: Fran Dios Date: Fri, 20 Sep 2024 16:05:33 +0900 Subject: [PATCH 2/7] Simplify regexps --- .../theme/src/cli/utilities/theme-environment/local-assets.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 8f4701fd24..c48fda3f42 100644 --- a/packages/theme/src/cli/utilities/theme-environment/local-assets.ts +++ b/packages/theme/src/cli/utilities/theme-environment/local-assets.ts @@ -72,8 +72,8 @@ function findLocalFile(event: H3Event, ctx: DevServerContex // Try to match theme asset files first and fallback to theme extension asset files return ( - tryGetFile(/^\/cdn\/.*?\/assets\/([^?]+)(\?|$)/, ctx.localThemeFileSystem) ?? - tryGetFile(/^\/ext\/cdn\/extensions\/.*?\/.*?\/assets\/([^?]+)(\?|$)/, ctx.localThemeExtensionFileSystem) ?? { + tryGetFile(/^\/cdn\/.*?\/assets\/([^?]+)/, ctx.localThemeFileSystem) ?? + tryGetFile(/^\/ext\/cdn\/extensions\/.*?\/assets\/([^?]+)/, ctx.localThemeExtensionFileSystem) ?? { isUnsynced: false, fileKey: undefined, file: undefined, From 0a3e863db58d9c43653f44e09a702cd701c24a11 Mon Sep 17 00:00:00 2001 From: Fran Dios Date: Fri, 20 Sep 2024 16:08:31 +0900 Subject: [PATCH 3/7] Test proxy cdn in JS files --- .../utilities/theme-environment/proxy.test.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts b/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts index 3f01315cfc..9b923afd41 100644 --- a/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts @@ -87,6 +87,24 @@ describe('dev proxy', () => { `) }) + test('proxies urls in JS files', () => { + const content = ` + console.log('https://cdn.shopify.com/path/to/assets/file1'); + const url = "https://cdn.shopify.com/path/to/assets/file1#zzz"; + fetch(\`https://cdn.shopify.com/path/to/assets/file1?q=123\`); + ` + + expect(injectCdnProxy(content, ctx)).toMatchInlineSnapshot( + ` + " + console.log('/cdn/path/to/assets/file1'); + const url = \\"/cdn/path/to/assets/file1#zzz\\"; + fetch(\`/cdn/path/to/assets/file1?q=123\`); + " + `, + ) + }) + test('proxies urls in Link header', () => { const linkHeader = `; rel="preconnect", ; rel="preconnect"; crossorigin,` + From 86744384fa12bf41cb57621f7d1a57820baf019f Mon Sep 17 00:00:00 2001 From: Fran Dios Date: Fri, 20 Sep 2024 16:23:05 +0900 Subject: [PATCH 4/7] Consider trailing spaces --- .../theme/src/cli/utilities/theme-environment/proxy.test.ts | 2 ++ packages/theme/src/cli/utilities/theme-environment/proxy.ts | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts b/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts index 9b923afd41..3e700f8932 100644 --- a/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts @@ -90,6 +90,7 @@ describe('dev proxy', () => { test('proxies urls in JS files', () => { const content = ` console.log('https://cdn.shopify.com/path/to/assets/file1'); + // Comment: https://cdn.shopify.com/path/to/assets/file1 something const url = "https://cdn.shopify.com/path/to/assets/file1#zzz"; fetch(\`https://cdn.shopify.com/path/to/assets/file1?q=123\`); ` @@ -98,6 +99,7 @@ describe('dev proxy', () => { ` " console.log('/cdn/path/to/assets/file1'); + // Comment: /cdn/path/to/assets/file1 something const url = \\"/cdn/path/to/assets/file1#zzz\\"; fetch(\`/cdn/path/to/assets/file1?q=123\`); " diff --git a/packages/theme/src/cli/utilities/theme-environment/proxy.ts b/packages/theme/src/cli/utilities/theme-environment/proxy.ts index bb1d9e2238..19247132d6 100644 --- a/packages/theme/src/cli/utilities/theme-environment/proxy.ts +++ b/packages/theme/src/cli/utilities/theme-environment/proxy.ts @@ -98,7 +98,7 @@ export function injectCdnProxy(originalContent: string, ctx: DevServerContext) { content = content.replace(vanityCdnRE, VANITY_CDN_PREFIX) // -- Only redirect usages of the main CDN for known local theme and theme extension assets to the local server: - const mainCdnRE = /(?:https?:)?\/\/cdn\.shopify\.com\/(.*?\/(assets\/[^?#"'`>]+))/g + const mainCdnRE = /(?:https?:)?\/\/cdn\.shopify\.com\/(.*?\/(assets\/[^?#"'`>\s]+))/g const filterAssets = (key: string) => key.startsWith('assets/') const existingAssets = new Set([...ctx.localThemeFileSystem.files.keys()].filter(filterAssets)) const existingExtAssets = new Set([...ctx.localThemeExtensionFileSystem.files.keys()].filter(filterAssets)) From 47051bbbe7e2f5804d1b6af3cef4cfb7f245e031 Mon Sep 17 00:00:00 2001 From: Fran Dios Date: Fri, 20 Sep 2024 19:28:46 +0900 Subject: [PATCH 5/7] Fix content-length in local assets for files with special characters --- .../cli/utilities/theme-environment/local-assets.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) 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 c48fda3f42..f82a87edd9 100644 --- a/packages/theme/src/cli/utilities/theme-environment/local-assets.ts +++ b/packages/theme/src/cli/utilities/theme-environment/local-assets.ts @@ -44,7 +44,15 @@ export function getAssetsHandler(_theme: Theme, ctx: DevServerContext) { return } - const fileContent = file.value ? injectCdnProxy(file.value, ctx) : Buffer.from(file.attachment ?? '', 'base64') + // Normalize the file content to a Buffer: + // - For attachments, we need to decode the base64 string. + // - For normal files, we need to get the real length of + // the file using Buffer to avoid issues with non-breaking + // spaces (NBSP, Unicode U+00A0), which have a different + // byte length than their visual representation. + const fileContent = file.value + ? Buffer.from(injectCdnProxy(file.value, ctx)) + : Buffer.from(file.attachment ?? '', 'base64') return serveStatic(event, { getContents: () => fileContent, From 964559140571f245d1119290fd2aef19679b12fa Mon Sep 17 00:00:00 2001 From: Fran Dios Date: Fri, 20 Sep 2024 19:29:12 +0900 Subject: [PATCH 6/7] Add test for serving local assets that contain non-breaking spaces --- .../theme-environment.test.ts | 35 ++++++++++++++++--- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts b/packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts index adafcf6fa2..183c5f3cbc 100644 --- a/packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts @@ -56,6 +56,15 @@ describe('setupDevServer', () => { value: '.another-class {}', }, ], + [ + 'assets/file-with-nbsp.js', + { + checksum: '1', + key: 'assets/file-with-nbsp.js', + // Contains a non-breaking space + value: 'const x = "Hello\u00A0World";', + }, + ], ]) const localThemeFileSystem = fakeThemeFileSystem('tmp', localFiles) @@ -165,10 +174,10 @@ describe('setupDevServer', () => { const dispatchEvent = async ( url: string, headers?: {[key: string]: string}, - ): Promise<{res: ServerResponse; status: number; body?: string}> => { + ): Promise<{res: ServerResponse; status: number; body: string | Buffer}> => { const event = createH3Event({url, headers}) const {res} = event.node - let body: string | undefined + let body: string | Buffer | undefined const resWrite = res.write.bind(res) res.write = (chunk) => { body ??= '' @@ -182,7 +191,7 @@ describe('setupDevServer', () => { } await server.dispatchEvent(event) - return {res, status: res.statusCode, body} + return {res, status: res.statusCode, body: body!} } test('mocks known endpoints', async () => { @@ -200,7 +209,25 @@ describe('setupDevServer', () => { const {res, body} = await eventPromise expect(res.getHeader('content-type')).toEqual('text/css') // The URL is proxied: - expect(body).toMatchInlineSnapshot(`".some-class { background: url(\\"/cdn/path/to/assets/file2.css\\") }"`) + expect(body.toString()).toMatchInlineSnapshot( + `".some-class { background: url(\\"/cdn/path/to/assets/file2.css\\") }"`, + ) + }) + + test('gets the right content for assets with non-breaking spaces', async () => { + const eventPromise = dispatchEvent('/cdn/somepathhere/assets/file-with-nbsp.js') + await expect(eventPromise).resolves.not.toThrow() + + expect(vi.mocked(render)).not.toHaveBeenCalled() + + const {res, body: bodyBuffer} = await eventPromise + const bodyString = bodyBuffer?.toString() ?? '' + expect(bodyString).toMatchInlineSnapshot(`"const x = \\"Hello\u00A0World\\";"`) + + // Ensure content-length contains the real length: + expect(bodyString.length).toEqual(24) + expect(bodyBuffer.length).toEqual(25) + expect(res.getHeader('content-length')).toEqual(25) }) test('renders HTML', async () => { From a1887fc14cc439815a6b65360e62887571a8823d Mon Sep 17 00:00:00 2001 From: Fran Dios Date: Fri, 20 Sep 2024 19:33:21 +0900 Subject: [PATCH 7/7] Changesets --- .changeset/smart-pots-drive.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/smart-pots-drive.md diff --git a/.changeset/smart-pots-drive.md b/.changeset/smart-pots-drive.md new file mode 100644 index 0000000000..a6bf16cdd2 --- /dev/null +++ b/.changeset/smart-pots-drive.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme': patch +--- + +Fix serving local assets that contain non-printable characters.