diff --git a/.changeset/perf-valid-scoped-css-class-session-cache.md b/.changeset/perf-valid-scoped-css-class-session-cache.md new file mode 100644 index 000000000..ccfa75498 --- /dev/null +++ b/.changeset/perf-valid-scoped-css-class-session-cache.md @@ -0,0 +1,6 @@ +--- +'@shopify/theme-check-common': patch +'@shopify/theme-language-server-common': patch +--- + +Restore `ValidScopedCSSClass` to the recommended config after fixing its save-latency regression on large themes. The check's per-file CSS class extraction now lives on the language server's document model, so it is memoized per file version and invalidated automatically when a file changes — no more full-theme rescan on every save. Also skips syntax-tree parsing for Liquid files that have no stylesheet tag. Resolves [#1179](https://github.com/Shopify/theme-tools/issues/1179) and reverses the temporary opt-out from [#1180](https://github.com/Shopify/theme-tools/pull/1180). diff --git a/packages/theme-check-common/src/checks/valid-scoped-css-class/index.ts b/packages/theme-check-common/src/checks/valid-scoped-css-class/index.ts index 5e8059ae6..4f043fa1c 100644 --- a/packages/theme-check-common/src/checks/valid-scoped-css-class/index.ts +++ b/packages/theme-check-common/src/checks/valid-scoped-css-class/index.ts @@ -76,7 +76,7 @@ export const ValidScopedCSSClass: LiquidCheckDefinition = { docs: { description: 'Reports CSS classes used in HTML class attributes that are not defined in any in-scope stylesheet tag or CSS asset file.', - recommended: false, + recommended: true, url: 'https://shopify.dev/docs/storefronts/themes/tools/theme-check/checks/valid-scoped-css-class', }, type: SourceCodeType.LiquidHtml, diff --git a/packages/theme-check-common/src/utils/styles.spec.ts b/packages/theme-check-common/src/utils/styles.spec.ts index 5c48e467b..75aa8adcf 100644 --- a/packages/theme-check-common/src/utils/styles.spec.ts +++ b/packages/theme-check-common/src/utils/styles.spec.ts @@ -4,8 +4,6 @@ import { extractCSSClassNames, extractCSSClassesFromLiquidUri, extractCSSClassesFromAssetUri, - extractCSSClassesFromAssets, - extractAllThemeCSSClasses, collectUsedClasses, collectUsedClassesFromSvg, } from './styles'; @@ -268,45 +266,3 @@ describe('extractCSSClassesFromAssetUri', () => { expect(classes.size).toBe(0); }); }); - -describe('extractCSSClassesFromAssets', () => { - it('collects classes from all .css files in assets', async () => { - const fs = new MockFileSystem({ - 'assets/base.css': '.base { color: red; }', - 'assets/theme.css': '.theme { color: blue; }', - 'assets/app.js': 'console.log("not css")', - }); - const toUri = (rel: string) => `file:///${rel}`; - const classes = await extractCSSClassesFromAssets(fs, toUri); - expect(classes).toEqual(new Set(['base', 'theme'])); - }); - - it('returns empty set when assets directory does not exist', async () => { - const fs = new MockFileSystem({}); - const toUri = (rel: string) => `file:///${rel}`; - const classes = await extractCSSClassesFromAssets(fs, toUri); - expect(classes.size).toBe(0); - }); -}); - -describe('extractAllThemeCSSClasses', () => { - it('collects classes from both liquid stylesheets and CSS assets', async () => { - const fs = new MockFileSystem({ - 'assets/theme.css': '.asset-class { color: red; }', - 'sections/section.liquid': - '{% stylesheet %} .section-class { color: blue; } {% endstylesheet %}', - 'snippets/snippet.liquid': - '{% stylesheet %} .snippet-class { font-size: 14px; } {% endstylesheet %}', - }); - const toUri = (rel: string) => `file:///${rel}`; - const classes = await extractAllThemeCSSClasses(fs, toUri); - expect(classes).toEqual(new Set(['asset-class', 'section-class', 'snippet-class'])); - }); - - it('returns empty set for empty theme', async () => { - const fs = new MockFileSystem({}); - const toUri = (rel: string) => `file:///${rel}`; - const classes = await extractAllThemeCSSClasses(fs, toUri); - expect(classes.size).toBe(0); - }); -}); diff --git a/packages/theme-check-common/src/utils/styles.ts b/packages/theme-check-common/src/utils/styles.ts index abf0e819e..315158f72 100644 --- a/packages/theme-check-common/src/utils/styles.ts +++ b/packages/theme-check-common/src/utils/styles.ts @@ -2,6 +2,7 @@ import { NodeTypes, TextNode, LiquidRawTag, + LiquidHtmlNode, toLiquidHtmlAST, AttributeNode, } from '@shopify/liquid-html-parser'; @@ -9,7 +10,6 @@ import safeParse from 'postcss-safe-parser'; import selectorParser from 'postcss-selector-parser'; import { SourceCodeType } from '../types'; import { AbstractFileSystem } from '../AbstractFileSystem'; -import { recursiveReadDirectory } from '../context-utils'; import { isValuedHtmlAttribute, ValuedHtmlAttribute } from '../checks/utils'; import { visit } from '../visitor'; @@ -33,6 +33,24 @@ export function extractCSSClassNames(css: string): Set { return classNames; } +/** Extract CSS class names from a Liquid AST's {% stylesheet %} tags. */ +export function extractCSSClassesFromLiquidAST(ast: LiquidHtmlNode): Set { + const classes = new Set(); + const cssStrings = visit(ast, { + LiquidRawTag(node: LiquidRawTag) { + if (node.name === 'stylesheet') { + return node.body.value; + } + }, + }); + for (const css of cssStrings) { + for (const cls of extractCSSClassNames(css)) { + classes.add(cls); + } + } + return classes; +} + /** Read a Liquid file and extract CSS class names from its {% stylesheet %} tags. */ export async function extractCSSClassesFromLiquidUri( uri: string, @@ -41,19 +59,15 @@ export async function extractCSSClassesFromLiquidUri( const classes = new Set(); try { const source = await fs.readFile(uri); + // Most liquid files have no {% stylesheet %} tag — skip the AST parse + // entirely when the tag isn't present. Saves ~20ms/file on large themes. + if (!/\{%-?\s*stylesheet/.test(source)) { + return classes; + } const ast = toLiquidHtmlAST(source); if (ast instanceof Error) return classes; - const cssStrings = visit(ast, { - LiquidRawTag(node: LiquidRawTag) { - if (node.name === 'stylesheet') { - return node.body.value; - } - }, - }); - for (const css of cssStrings) { - for (const cls of extractCSSClassNames(css)) { - classes.add(cls); - } + for (const cls of extractCSSClassesFromLiquidAST(ast)) { + classes.add(cls); } } catch { // File not found or parse error — skip @@ -74,60 +88,6 @@ export async function extractCSSClassesFromAssetUri( } } -/** Collect all CSS class names from all .css files in the assets directory. */ -export async function extractCSSClassesFromAssets( - fs: AbstractFileSystem, - toUri: (relativePath: string) => string, -): Promise> { - const classes = new Set(); - try { - const assetsUri = toUri('assets'); - const files = await fs.readDirectory(assetsUri); - const cssFiles = files.filter(([uri]) => uri.endsWith('.css')); - const results = await Promise.all( - cssFiles.map(([uri]) => extractCSSClassesFromAssetUri(uri, fs)), - ); - for (const fileClasses of results) { - for (const cls of fileClasses) { - classes.add(cls); - } - } - } catch { - // assets directory might not exist - } - return classes; -} - -/** Collect ALL CSS classes defined anywhere in the theme (all liquid stylesheet tags + CSS assets). */ -export async function extractAllThemeCSSClasses( - fs: AbstractFileSystem, - toUri: (relativePath: string) => string, -): Promise> { - const allClasses = new Set(); - - // Collect from CSS asset files - const assetClasses = await extractCSSClassesFromAssets(fs, toUri); - for (const cls of assetClasses) allClasses.add(cls); - - // Collect from all liquid files' stylesheet tags - try { - const rootUri = toUri(''); - const liquidFiles = await recursiveReadDirectory(fs, rootUri, ([uri]) => - uri.endsWith('.liquid'), - ); - const results = await Promise.all( - liquidFiles.map((uri) => extractCSSClassesFromLiquidUri(uri, fs)), - ); - for (const classes of results) { - for (const cls of classes) allClasses.add(cls); - } - } catch { - // root directory read failure - } - - return allClasses; -} - /** Case-insensitive check for the `class` attribute (HTML attributes are case-insensitive). */ function isClassAttribute(attr: ValuedHtmlAttribute): boolean { return ( diff --git a/packages/theme-check-node/configs/recommended.yml b/packages/theme-check-node/configs/recommended.yml index c93f4f479..adaa03fdf 100644 --- a/packages/theme-check-node/configs/recommended.yml +++ b/packages/theme-check-node/configs/recommended.yml @@ -177,6 +177,9 @@ ValidSchemaName: ValidSchemaTranslations: enabled: true severity: 0 +ValidScopedCSSClass: + enabled: true + severity: 1 ValidSettingsKey: enabled: true severity: 0 diff --git a/packages/theme-language-server-common/src/diagnostics/runChecks.ts b/packages/theme-language-server-common/src/diagnostics/runChecks.ts index 4918dc1cb..1fcf5afa4 100644 --- a/packages/theme-language-server-common/src/diagnostics/runChecks.ts +++ b/packages/theme-language-server-common/src/diagnostics/runChecks.ts @@ -79,11 +79,20 @@ export function makeRunChecks( return themeGraphManager.getDependencies(uri); }, - async getCSSClassesForURI(uri: string): Promise> { - if (uri.endsWith('.css')) { - return extractCSSClassesFromAssetUri(uri, fs); - } - return extractCSSClassesFromLiquidUri(uri, fs); + getCSSClassesForURI(uri: string): Promise> { + // Liquid files tracked by the DocumentManager already have their AST + // parsed; getCSSClasses() is memoized per file version there, so it + // is reused across saves and invalidated automatically when the + // document changes. + const doc = documentManager.get(uri); + if (doc?.type === SourceCodeType.LiquidHtml) return doc.getCSSClasses(); + // CSS assets aren't tracked by the DocumentManager, and occasionally + // we get asked about liquid URIs outside the preloaded set (e.g. + // before preload finishes). Fall back to direct parsing in both + // cases. + return uri.endsWith('.css') + ? extractCSSClassesFromAssetUri(uri, fs) + : extractCSSClassesFromLiquidUri(uri, fs); }, // TODO should do something for app blocks? diff --git a/packages/theme-language-server-common/src/documents/DocumentManager.ts b/packages/theme-language-server-common/src/documents/DocumentManager.ts index e12491376..21c1b2790 100644 --- a/packages/theme-language-server-common/src/documents/DocumentManager.ts +++ b/packages/theme-language-server-common/src/documents/DocumentManager.ts @@ -1,6 +1,7 @@ import { AbstractFileSystem, assertNever, + extractCSSClassesFromLiquidAST, memoize, path, recursiveReadDirectory, @@ -217,6 +218,17 @@ export class DocumentManager { return extractDocDefinition(uri, ast); }), + /** + * Lazy and only computed once per file version. Returns the set of + * CSS class selectors declared in this file's {% stylesheet %} tags. + * Shared by ValidScopedCSSClass and any future feature that needs to + * know which classes a file defines. + */ + getCSSClasses: memo(async () => { + const ast = sourceCode.ast; + if (isError(ast)) return new Set(); + return extractCSSClassesFromLiquidAST(ast); + }), }; default: return assertNever(sourceCode); diff --git a/packages/theme-language-server-common/src/documents/types.ts b/packages/theme-language-server-common/src/documents/types.ts index af9ed4e10..653879e30 100644 --- a/packages/theme-language-server-common/src/documents/types.ts +++ b/packages/theme-language-server-common/src/documents/types.ts @@ -25,6 +25,7 @@ export type AugmentedJsonSourceCode = _AugmentedSourceCode; export type AugmentedLiquidSourceCode = _AugmentedSourceCode & { getSchema: () => Promise; getLiquidDoc: () => Promise; + getCSSClasses: () => Promise>; }; /**