Skip to content

Commit b422c22

Browse files
karreirokarreiro
authored
Merge pull request #6125 from Shopify/fix-theme-dev-performance
Fix performance issue on `shopify theme dev` command
2 parents 1d40ea4 + 0f566d1 commit b422c22

File tree

8 files changed

+185
-51
lines changed

8 files changed

+185
-51
lines changed

‎.changeset/angry-comics-greet.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@shopify/theme': patch
3+
---
4+
5+
Fix performance issue on `shopify theme dev` command

‎packages/theme/package.json‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,9 +54,6 @@
5454
"@vitest/coverage-istanbul": "^3.1.4",
5555
"node-stream-zip": "^1.15.0"
5656
},
57-
"peerDependencies": {
58-
"@shopify/liquid-html-parser": "^2.8.2"
59-
},
6057
"engines": {
6158
"node": ">=20.10.0"
6259
},

‎packages/theme/src/cli/utilities/theme-environment/hot-reload/server.test.ts‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -447,12 +447,6 @@ describe('getUpdatedFileParts', () => {
447447

448448
const newFileResult = getUpdatedFileParts(file)
449449

450-
expect(output.outputDebug).toHaveBeenCalledWith(
451-
expect.stringContaining(
452-
'Error parsing Liquid file "sections/broken.liquid" to detect updated file parts. LiquidHTMLParsingError:',
453-
),
454-
)
455-
456450
expect(newFileResult?.stylesheetTag).toBe(true)
457451
expect(newFileResult?.javascriptTag).toBe(false)
458452
expect(newFileResult?.schemaTag).toBe(false)

‎packages/theme/src/cli/utilities/theme-environment/hot-reload/server.ts‎

Lines changed: 4 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {patchRenderingResponse} from '../proxy.js'
44
import {createFetchError, extractFetchErrorInfo} from '../../errors.js'
55
import {inferLocalHotReloadScriptPath} from '../../theme-fs.js'
66
import {parseServerEvent} from '../server-utils.js'
7+
import {getLiquidTagContent} from '../liquid-tag-content.js'
78
import {
89
createError,
910
createEventStream,
@@ -17,8 +18,6 @@ import {renderError, renderInfo, renderWarning} from '@shopify/cli-kit/node/ui'
1718
import {extname, joinPath} from '@shopify/cli-kit/node/path'
1819
import {parseJSON} from '@shopify/theme-check-node'
1920
import {readFile} from '@shopify/cli-kit/node/fs'
20-
import {NodeTypes, toLiquidHtmlAST, walk} from '@shopify/liquid-html-parser'
21-
import {outputDebug} from '@shopify/cli-kit/node/output'
2221
import EventEmitter from 'node:events'
2322
import type {
2423
HotReloadEvent,
@@ -453,25 +452,9 @@ export function getUpdatedFileParts(file: ThemeAsset) {
453452
cacheEntry[tagName] = content
454453
}
455454

456-
try {
457-
walk(toLiquidHtmlAST(file.value), (node) => {
458-
if (node.type !== NodeTypes.LiquidRawTag) return
459-
460-
const nodeName = node.name as LiquidTag
461-
if (liquidTags.includes(nodeName)) {
462-
handleTagMatch(nodeName, node.body.value)
463-
}
464-
})
465-
// eslint-disable-next-line no-catch-all/no-catch-all
466-
} catch (err: unknown) {
467-
const error = err as Error
468-
outputDebug(`Error parsing Liquid file "${file.key}" to detect updated file parts. ${error.stack ?? error.message}`)
469-
470-
for (const tag of liquidTags) {
471-
const tagRE = new RegExp(`{%\\s*${tag}\\s*%}([^%]*){%\\s*end${tag}\\s*%}`)
472-
const match = otherContent.match(tagRE)?.[1]
473-
if (match) handleTagMatch(tag, match)
474-
}
455+
for (const tag of liquidTags) {
456+
const match = getLiquidTagContent(file.value ?? '', tag)
457+
if (match) handleTagMatch(tag, match)
475458
}
476459

477460
otherContent = normalizeContent(

‎packages/theme/src/cli/utilities/theme-environment/liquid-tag-content.test.ts‎

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
import {getLiquidTagContent} from './liquid-tag-content.js'
2+
import {describe, expect, test} from 'vitest'
3+
4+
const scenarios = [
5+
{
6+
name: 'handles simple cases with all tags present',
7+
fileContent: `
8+
{% stylesheet %}
9+
.foo { color: red; }
10+
{% endstylesheet %}
11+
{% javascript %}
12+
console.log('hi');
13+
{% endjavascript %}
14+
{% schema %}
15+
{ "name": "Test" }
16+
{% endschema %}
17+
<div>Other content</div>
18+
`,
19+
expected: {
20+
stylesheet: '.foo { color: red; }',
21+
javascript: "console.log('hi');",
22+
schema: '{ "name": "Test" }',
23+
},
24+
},
25+
{
26+
name: 'handles simple cases with extra whitespace',
27+
fileContent: `
28+
{% stylesheet %}
29+
.baz { color: green; }
30+
{% endstylesheet %}
31+
{%javascript%}
32+
alert('hi');
33+
{%endjavascript%}
34+
{% schema %}
35+
{ "name": "Whitespace" }
36+
{% endschema %}
37+
`,
38+
expected: {
39+
stylesheet: '.baz { color: green; }',
40+
javascript: "alert('hi');",
41+
schema: '{ "name": "Whitespace" }',
42+
},
43+
},
44+
{
45+
name: 'handles dash cases with all tags present',
46+
fileContent: `
47+
{%- stylesheet -%}
48+
.foo { color: red; }
49+
{%- endstylesheet -%}
50+
{%- javascript -%}
51+
console.log('hi');
52+
{%- endjavascript -%}
53+
{%- schema -%}
54+
{ "name": "Test" }
55+
{%- endschema -%}
56+
<div>Other content</div>
57+
`,
58+
expected: {
59+
stylesheet: '.foo { color: red; }',
60+
javascript: "console.log('hi');",
61+
schema: '{ "name": "Test" }',
62+
},
63+
},
64+
{
65+
name: 'detects dash cases with all tags present and whitespace',
66+
fileContent: `
67+
{%- stylesheet -%}
68+
.foo { color: red; }
69+
{%- endstylesheet -%}
70+
{% javascript %}
71+
console.log('hi');
72+
{%- endjavascript -%}
73+
{% schema -%}
74+
{ "name": "Test" }
75+
{%- endschema -%}
76+
<div>Other content</div>
77+
`,
78+
expected: {
79+
stylesheet: '.foo { color: red; }',
80+
javascript: "console.log('hi');",
81+
schema: '{ "name": "Test" }',
82+
},
83+
},
84+
{
85+
name: 'handles files when only the stylesheet tag is present',
86+
fileContent: `
87+
{% stylesheet %}
88+
.bar { color: blue; }
89+
{% endstylesheet %}
90+
<div>No other tags</div>
91+
`,
92+
expected: {
93+
stylesheet: '.bar { color: blue; }',
94+
javascript: '',
95+
schema: '',
96+
},
97+
},
98+
{
99+
name: 'handles files when no tags are present',
100+
fileContent: `
101+
<div>Just HTML, no tags</div>
102+
`,
103+
expected: {
104+
stylesheet: '',
105+
javascript: '',
106+
schema: '',
107+
},
108+
},
109+
]
110+
111+
describe('getLiquidTagContent', () => {
112+
for (const scenario of scenarios) {
113+
test(scenario.name, () => {
114+
for (const tag of ['stylesheet', 'javascript', 'schema'] as const) {
115+
const expected = scenario.expected[tag]
116+
const actual = getLiquidTagContent(scenario.fileContent, tag) ?? ''
117+
118+
expect(actual.trim()).toBe(expected.trim())
119+
}
120+
})
121+
}
122+
})

‎packages/theme/src/cli/utilities/theme-environment/liquid-tag-content.ts‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
/**
2+
* Extracts the content of a given Liquid tag.
3+
*
4+
* This function is designed for high-throughput, runtime asset compilation in
5+
* large Shopify theme assets.
6+
*
7+
* All theme pages require locally-built compiled JavaScript and CSS files, so
8+
* extraction must be extremely fast. Previously, `@shopify/liquid-html-parser`
9+
* was used, but while is more robust and accurate approach parse times
10+
* exceeded 1ms per file (in local benchmarks), which is prohibitive for large
11+
* themes:
12+
*
13+
* file size: 0.29 KB - parse time: 1.38ms
14+
* file size: 0.35 KB - parse time: 1.44ms
15+
* file size: 6.37 KB - parse time: 26.16ms
16+
* file size: 11.72 KB - parse time: 33.33ms
17+
* file size: 21.15 KB - parse time: 70.30ms
18+
* file size: 134.66 KB - parse time: 258.89ms
19+
* file size: 250.00 KB - parse time: 506.89ms
20+
*
21+
* This function adopts the regular expression approach, keeping extraction
22+
* well under ~1ms per file, which is crucial for developer experience and
23+
* runtime performance.
24+
*
25+
* @param liquid - The full Liquid template as a string.
26+
* @param tag - The Liquid tag to extract content from.
27+
*
28+
* @returns The tag content, or undefined if not found.
29+
*/
30+
export function getLiquidTagContent(liquid: string, tag: 'javascript' | 'stylesheet' | 'schema'): string | undefined {
31+
const openTagRE = new RegExp(`{%-?\\s*${tag}\\s*-?%}`)
32+
const closeTagRE = new RegExp(`{%-?\\s*end${tag}\\s*-?%}`)
33+
34+
const openMatch = openTagRE.exec(liquid)
35+
if (!openMatch) {
36+
return
37+
}
38+
39+
const startIndex = openMatch.index + openMatch[0].length
40+
closeTagRE.lastIndex = startIndex
41+
42+
const closeMatch = closeTagRE.exec(liquid)
43+
if (!closeMatch) {
44+
return
45+
}
46+
47+
const endIndex = closeMatch.index
48+
49+
return liquid.slice(startIndex, endIndex)
50+
}

‎packages/theme/src/cli/utilities/theme-environment/local-assets.ts‎

Lines changed: 4 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,9 @@
11
import {injectCdnProxy} from './proxy.js'
22
import {parseServerEvent} from './server-utils.js'
3+
import {getLiquidTagContent} from './liquid-tag-content.js'
34
import {lookupMimeType} from '@shopify/cli-kit/node/mimes'
45
import {defineEventHandler, H3Event, serveStatic, setResponseHeader, sendError, createError} from 'h3'
56
import {joinPath} from '@shopify/cli-kit/node/path'
6-
import {toLiquidHtmlAST, walk, NodeTypes} from '@shopify/liquid-html-parser'
7-
import {outputDebug} from '@shopify/cli-kit/node/output'
87
import type {Theme, ThemeAsset, VirtualFileSystem} from '@shopify/cli-kit/node/themes/types'
98
import type {DevServerContext} from './types.js'
109

@@ -221,22 +220,9 @@ function getTagContent(file: ThemeAsset, tag: 'javascript' | 'stylesheet') {
221220

222221
const contents = [`/* ${file.key} */`]
223222

224-
try {
225-
walk(toLiquidHtmlAST(file.value), (node) => {
226-
if (node.type === NodeTypes.LiquidRawTag && node.name === tag) {
227-
contents.push(node.body.value)
228-
}
229-
})
230-
// eslint-disable-next-line no-catch-all/no-catch-all
231-
} catch (err: unknown) {
232-
const error = err as Error
233-
outputDebug(`Error parsing Liquid file "${file.key}" to extract ${tag} tag. ${error.stack ?? error.message}`)
234-
235-
const tagRE = new RegExp(`{%\\s*${tag}\\s*%}([^%]*){%\\s*end${tag}\\s*%}`)
236-
const tagContent = file.value?.match(tagRE)?.[1]
237-
if (tagContent) {
238-
contents.push(tagContent)
239-
}
223+
const tagContent = getLiquidTagContent(file.value ?? '', tag)
224+
if (tagContent) {
225+
contents.push(tagContent)
240226
}
241227

242228
if (contents.length > 1) {

‎pnpm-lock.yaml‎

Lines changed: 0 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)