Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/perf-valid-scoped-css-class-session-cache.md
Original file line number Diff line number Diff line change
@@ -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).
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
44 changes: 0 additions & 44 deletions packages/theme-check-common/src/utils/styles.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import {
extractCSSClassNames,
extractCSSClassesFromLiquidUri,
extractCSSClassesFromAssetUri,
extractCSSClassesFromAssets,
extractAllThemeCSSClasses,
collectUsedClasses,
collectUsedClassesFromSvg,
} from './styles';
Expand Down Expand Up @@ -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);
});
});
92 changes: 26 additions & 66 deletions packages/theme-check-common/src/utils/styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ import {
NodeTypes,
TextNode,
LiquidRawTag,
LiquidHtmlNode,
toLiquidHtmlAST,
AttributeNode,
} from '@shopify/liquid-html-parser';
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';

Expand All @@ -33,6 +33,24 @@ export function extractCSSClassNames(css: string): Set<string> {
return classNames;
}

/** Extract CSS class names from a Liquid AST's {% stylesheet %} tags. */
export function extractCSSClassesFromLiquidAST(ast: LiquidHtmlNode): Set<string> {
const classes = new Set<string>();
const cssStrings = visit<SourceCodeType.LiquidHtml, string>(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,
Expand All @@ -41,19 +59,15 @@ export async function extractCSSClassesFromLiquidUri(
const classes = new Set<string>();
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<SourceCodeType.LiquidHtml, string>(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
Expand All @@ -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<Set<string>> {
const classes = new Set<string>();
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<Set<string>> {
const allClasses = new Set<string>();

// 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;
}

Comment thread
aswamy marked this conversation as resolved.
/** Case-insensitive check for the `class` attribute (HTML attributes are case-insensitive). */
function isClassAttribute(attr: ValuedHtmlAttribute): boolean {
return (
Expand Down
3 changes: 3 additions & 0 deletions packages/theme-check-node/configs/recommended.yml
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,9 @@ ValidSchemaName:
ValidSchemaTranslations:
enabled: true
severity: 0
ValidScopedCSSClass:
enabled: true
severity: 1
ValidSettingsKey:
enabled: true
severity: 0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,20 @@ export function makeRunChecks(
return themeGraphManager.getDependencies(uri);
},

async getCSSClassesForURI(uri: string): Promise<Set<string>> {
if (uri.endsWith('.css')) {
return extractCSSClassesFromAssetUri(uri, fs);
}
return extractCSSClassesFromLiquidUri(uri, fs);
getCSSClassesForURI(uri: string): Promise<Set<string>> {
// 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?
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {
AbstractFileSystem,
assertNever,
extractCSSClassesFromLiquidAST,
memoize,
path,
recursiveReadDirectory,
Expand Down Expand Up @@ -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<string>();
return extractCSSClassesFromLiquidAST(ast);
}),
};
default:
return assertNever(sourceCode);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export type AugmentedJsonSourceCode = _AugmentedSourceCode<SourceCodeType.JSON>;
export type AugmentedLiquidSourceCode = _AugmentedSourceCode<SourceCodeType.LiquidHtml> & {
getSchema: () => Promise<SectionSchema | ThemeBlockSchema | AppBlockSchema | undefined>;
getLiquidDoc: () => Promise<DocDefinition | undefined>;
getCSSClasses: () => Promise<Set<string>>;
};

/**
Expand Down
Loading