8000 Gracefully handle files that are not from a theme by charlespwd · Pull Request #985 · Shopify/theme-tools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Gracefully handle files that are not from a theme #985

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 17, 2025
Merged

Gracefully handle files that are not from a theme #985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions .changeset/violet-cycles-yawn.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
'@shopify/theme-language-server-common': patch
'@shopify/theme-language-server-node': patch
'@shopify/theme-check-common': patch
'theme-check-vscode': patch
---

Gracefully handle files that are not part of a theme
16 changes: 3 additions & 13 deletions packages/theme-check-common/src/checks/valid-visible-if/visible-if-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,6 @@
import {
type LiquidVariableLookup,
toLiquidHtmlAST,
NodeTypes,
LiquidVariableOutput,
} from '@shopify/liquid-html-parser';
import { type LiquidVariableLookup, NodeTypes, toLiquidHtmlAST } from '@shopify/liquid-html-parser';
import { Context, SourceCodeType } from '../..';
import { findRoot } from '../../find-root';
import { parseJSON } from '../../json';
import { join } from '../../path';
import { visit } from '../../visitor';

export type Vars = { [key: string]: Vars | true };
Expand Down Expand Up @@ -140,11 +133,8 @@ export async function getGlobalSettings(context: Context<SourceCodeType>) {
const globalSettings: string[] = [];

try {
const path = join(
await findRoot(context.file.uri, context.fileExists),
'config/settings_schema.json',
);
const settingsFile = await context.fs.readFile(path);
Comment on lines -143 to -147
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toURI does this better.

const uri = context.toUri('config/settings_schema.json');
const settingsFile = await context.fs.readFile(uri);
const settings = parseJSON(settingsFile);
if (Array.isArray(settings)) {
for (const group of settings) {
Expand Down
17 changes: 12 additions & 5 deletions packages/theme-check-common/src/find-root.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,19 @@ async function isRoot(dir: UriString, fileExists: FileExists) {
return or(
fileExists(path.join(dir, 'shopify.extension.toml')), // for theme-app-extensions
fileExists(path.join(dir, '.theme-check.yml')),
fileExists(path.join(dir, '.git')),

// zip files and TAEs might not have config files, but they should have a
// snippets directory but in case they do specify a .theme-check.yml a
// Maybe the root of the workspace has an assets + snippets directory, we'll accept that
and(
fileExists(path.join(dir, '.git')),
fileExists(path.join(dir, 'assets')),
fileExists(path.join(dir, 'snippets')),
),

// zip files and TAEs might not have config files, but they should have an
// assets & snippets directory but in case they do specify a .theme-check.yml a
// couple of directories up, we should respect that
and(
fileExists(path.join(dir, 'assets')),
fileExists(path.join(dir, 'snippets')),
not(fileExists(path.join(path.dirname(dir), '.theme-check.yml'))),
not(fileExists(path.join(path.dirname(path.dirname(dir)), '.theme-check.yml'))),
Expand Down Expand Up @@ -49,7 +56,7 @@ async function not(ap: Promise<boolean>) {
* Note: that this is not the theme root. The config file might have a `root` entry in it
* that points to somewhere else.
*/
export async function findRoot(curr: UriString, fileExists: FileExists): Promise<UriString> {
export async function findRoot(curr: UriString, fileExists: FileExists): Promise<UriString | null> {
const currIsRoot = await isRoot(curr, fileExists);
if (currIsRoot) {
return curr;
Expand All @@ -58,7 +65,7 @@ export async function findRoot(curr: UriString, fileExists: FileExists): Promise
const dir = path.dirname(curr);
const currIsAbsoluteRoot = dir === curr;
if (currIsAbsoluteRoot) {
return curr;
return null; // Root not found.
}

return findRoot(dir, fileExists);
Expand Down
2 changes: 1 addition & 1 deletion packages/theme-check-common/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ function createContext<T extends SourceCodeType, S extends Schema>(
...dependencies,
validateJSON,
settings: createSettings(checkSettings, check.meta.schema),
toUri: (relativePath) => path.join(config.rootUri, relativePath),
toUri: (relativePath) => path.join(config.rootUri, ...relativePath.split('/')),
toRelativePath: (uri) => path.relative(uri, config.rootUri),
report(problem: Problem<T>): void {
offenses.push({
Expand Down
53 changes: 30 additions & 23 deletions packages/theme-check-common/src/test/MockFileSystem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { AbstractFileSystem, FileStat, FileTuple, FileType } from '../AbstractFi
import { deepGet } from '../utils';
import { normalize, relative } from '../path';
import { MockTheme } from './MockTheme';
import * as path from '../path';

interface FileTree {
[fileName: string]: string | FileTree;
Expand All @@ -24,14 +25,15 @@ export class MockFileSystem implements AbstractFileSystem {
}

async readDirectory(uri: string): Promise<FileTuple[]> {
// eslint-disable-next-line no-param-reassign
uri = uri.replace(/\/$/, '');
const relativePath = this.rootRelative(uri);
const tree =
normalize(uri) === this.rootUri
path.normalize(uri) === this.rootUri
? this.fileTree
: deepGet(this.fileTree, relativePath.split('/'));
if (tree === undefined) {
throw new Error(`Directory not found: ${uri} in ${this.rootUri}`);
if (tree === undefined || tree === null) {
throw new Error(`Directory not found: ${uri} for ${this.rootUri}`);
}

if (typeof tree === 'string') {
Expand All @@ -46,32 +48,37 @@ export class MockFileSystem implements AbstractFileSystem {
async stat(uri: string): Promise<FileStat> {
const relativePath = this.rootRelative(uri);
const source = this.mockTheme[relativePath];
if (source === undefined) {
throw new Error('File not found');
if (source) {
return {
type: FileType.File,
size: source.length ?? 0,
};
}
return {
type: FileType.File,
size: source.length ?? 0,
};
}

private _fileTree: FileTree | null = null;
const readdirResult = await this.readDirectory(uri);
if (readdirResult) {
return {
type: FileType.Directory,
size: 0, // Size is not applicable for directories
};
}

throw new Error(`File not found: ${uri} for ${this.rootUri}`);
}

private get fileTree(): FileTree {
if (!this._fileTree) {
this._fileTree = {};
for (const [relativePath, source] of Object.entries(this.mockTheme)) {
const segments = relativePath.split('/');
let current = this._fileTree;
for (let i = 0; i < segments.length - 1; i++) {
const segment = segments[i];
current[segment] ??= {};
current = current[segment] as FileTree;
}
current[segments[segments.length - 1]] = source;
const result: FileTree = {};
for (const [relativePath, source] of Object.entries(this.mockTheme)) {
const segments = relativePath.split('/');
let current = result;
for (let i = 0; i < segments.length - 1; i++) {
const segment = segments[i];
current[segment] ??= {};
current = current[segment] as FileTree;
}
current[segments[segments.length - 1]] = source;
}
return this._fileTree;
return result;
Comment on lines +70 to +81
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to remove memoization because of a test that mutated this.mockTheme. It's only for tests, perf doesn't matter :)

}

private rootRelative(uri: string) {
Expand Down
2 changes: 1 addition & 1 deletion packages/theme-check-common/src/test/test-helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export async function check(
const isValidSchema = async () => true;

const defaultMockDependencies: Dependencies = {
fs: new MockFileSystem(themeDesc),
fs: new MockFileSystem({ '.theme-check.yml': '', ...themeDesc }),
async getBlockSchema(name) {
const block = blocks.get(name);
if (!block) return undefined;
Expand Down
10000
4 changes: 4 additions & 0 deletions packages/theme-check-node/src/find-root.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ import { NodeFileSystem } from './NodeFileSystem';
import { makeTempWorkspace, Workspace } from './test/test-helpers';

const theme = {
assets: {
'theme.js': '',
'theme.css': '',
},
locales: {
'en.default.json': JSON.stringify({ beverage: 'coffee' }),
'fr.json': '{}',
Expand Down
96 changes: 54 additions & 42 deletions packages/theme-language-server-common/src/diagnostics/runChecks.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {
AbstractFileSystem,
allChecks,
LiquidCheckDefinition,
path,
Expand Down Expand Up @@ -44,6 +45,9 @@ describe('Module: runChecks', () => {
let documentManager: DocumentManager;
let connection: { sendDiagnostics: ReturnType<typeof vi.fn> };
let runChecks: ReturnType<typeof makeRunChecks>;
let fs: MockFileSystem;
const rootUri = path.normalize('browser:///theme');
const fileUri = path.join(rootUri, 'snippets', 'input.liquid');

beforeEach(() => {
connection = {
Expand All @@ -52,13 +56,20 @@ describe('Module: runChecks', () => {

documentManager = new DocumentManager();
diagnosticsManager = new DiagnosticsManager(connection as any as Connection);
fs = new MockFileSystem(
{
'.theme-check.yml': '',
'snippets/input.liquid': `{{ 'any' | filter }}`,
},
rootUri,
);
runChecks = makeRunChecks(documentManager, diagnosticsManager, {
fs: new MockFileSystem({}, 'browser:/'),
fs,
loadConfig: async () => ({
context: 'theme',
context: 'theme' as const,
settings: {},
checks: [LiquidFilter],
rootUri: 'browser:/',
rootUri,
}),
themeDocset: {
filters: async () => [],
Expand All @@ -74,15 +85,14 @@ describe('Module: runChecks', () => {
});

it('should send diagnostics when there are errors', async () => {
const fileURI = 'browser:/input.liquid';
const fileContents = `{{ 'any' | filter }}`;
const fileContents = await fs.readFile(fileUri);
const fileVersion = 0;
documentManager.open(fileURI, fileContents, fileVersion);
documentManager.open(fileUri, fileContents, fileVersion);

await runChecks(['browser:/input.liquid']);
await runChecks([fileUri]);
expect(connection.sendDiagnostics).toBeCalled();
expect(connection.sendDiagnostics).toBeCalledWith({
uri: fileURI,
uri: fileUri,
version: fileVersion,
diagnostics: [
{
Expand All @@ -106,23 +116,22 @@ describe('Module: runChecks', () => {
});

it('should send an empty array when the errors were cleared', async () => {
const fileURI = 'browser:/input.liquid';
const fileContentsWithError = `{{ 'any' | filter }}`;
const fileContentsWithoutError = `{{ 'any' }}`;
let fileVersion = 1;

// Open and have errors
documentManager.open(fileURI, fileContentsWithError, fileVersion);
await runChecks(['browser:/input.liquid']);
documentManager.open(fileUri, fileContentsWithError, fileVersion);
await runChecks([fileUri]);

// Change doc to fix errors
fileVersion = 2;
documentManager.change(fileURI, fileContentsWithoutError, fileVersion);
await runChecks(['browser:/input.liquid']);
documentManager.change(fileUri, fileContentsWithoutError, fileVersion);
await runChecks([fileUri]);

expect(connection.sendDiagnostics).toBeCalledTimes(2);
expect(connection.sendDiagnostics).toHaveBeenLastCalledWith({
uri: fileURI,
uri: fileUri,
version: fileVersion,
diagnostics: [],
});
Expand All @@ -131,7 +140,7 @@ describe('Module: runChecks', () => {
it('should send diagnostics per URI when there are errors', async () => {
const files = [
{
fileURI: 'browser:/input1.liquid',
fileURI: path.join(rootUri, 'snippets', 'input1.liquid'),
fileContents: `{{ 'any' | filter }}`,
fileVersion: 0,
diagnostics: [
Expand All @@ -154,7 +163,7 @@ describe('Module: runChecks', () => {
],
},
{
fileURI: 'browser:/input2.liquid',
fileURI: path.join(rootUri, 'snippets', 'input2.liquid'),
// same but on a new line
fileContents: `\n{{ 'any' | filter }}`,
fileVersion: 0,
Expand Down Expand Up @@ -183,7 +192,7 @@ describe('Module: runChecks', () => {
documentManager.open(fileURI, fileContents, fileVersion);
});

await runChecks(['browser:/input1.liquid']);
await runChecks([path.join(rootUri, 'snippets', 'input1.liquid')]);

files.forEach(({ fileURI, fileVersion, diagnostics }) => {
expect(connection.sendDiagnostics).toBeCalledWith({
Expand All @@ -196,23 +205,24 @@ describe('Module: runChecks', () => {

it('should use the contents of the default translations file buffer (if any) instead of the result of the factory', async () => {
const defaultPath = 'locales/en.default.json';
const defaultURI = `browser:/${defaultPath}`;
const defaultURI = path.join(rootUri, ...defaultPath.split('/'));
const frPath = 'locales/fr.json';
const frURI = `browser:/${frPath}`;
const frURI = path.join(rootUri, ...frPath.split('/'));
const files = {
'.theme-check.yml': '',
[defaultPath]: JSON.stringify({ hello: 'hello' }),
[frPath]: JSON.stringify({ hello: 'bonjour', hi: 'salut' }),
};

const matchingTranslation = allChecks.filter((c) => c.meta.code === 'MatchingTranslations');
expect(matchingTranslation).to.have.lengthOf(1);
runChecks = makeRunChecks(documentManager, diagnosticsManager, {
fs: new MockFileSystem(files, path.normalize('browser:/')),
fs: new MockFileSystem(files, rootUri),
loadConfig: async () => ({
context: 'theme',
settings: {},
checks: matchingTranslation,
rootUri: path.normalize('browser:/'),
rootUri: rootUri,
}),
themeDocset: {
filters: async () => [],
Expand All @@ -229,29 +239,31 @@ describe('Module: runChecks', () => {
// Open and have errors
documentManager.open(frURI, files[frPath], 0);
await runChecks([frURI]);
expect(connection.sendDiagnostics).toHaveBeenCalledWith({
uri: frURI,
version: 0,
diagnostics: [
{
source: 'theme-check',
code: 'MatchingTranslations',
codeDescription: { href: expect.any(String) },
message: `A default translation for 'hi' does not exist`,
severity: 1,
range: {
end: {
character: 31,
line: 0,
},
start: {
character: 19,
line: 0,
expect(connection.sendDiagnostics).toHaveBeenCalledWith(
expect.objectContaining({
uri: frURI,
version: 0,
diagnostics: expect.arrayContaining([
{
source: 'theme-check',
code: 'MatchingTranslations',
codeDescription: { href: expect.any(String) },
message: `A default translation for 'hi' does not exist`,
severity: 1,
range: {
end: {
character: 31,
line: 0,
},
start: {
character: 19,
line: 0,
},
},
},
},
],
});
]),
}),
);

// Change the contents of the defaultURI buffer, expect frURI to be fixed
documentManager.open(defaultURI, files[defaultPath], 0);
Expand Down
2 changes: 1 addition & 1 deletion packages/theme-language-server-common/src/diagnostics/runChecks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export function makeRunChecks(
// then we recheck theme1
const fileExists = makeFileExists(fs);
const rootURIs = await Promise.all(triggerURIs.map((uri) => findRoot(uri, fileExists)));
const deduplicatedRootURIs = new Set(rootURIs);
const deduplicatedRootURIs = new Set<string>(rootURIs.filter((x): x is string => !!x));
await Promise.all([...deduplicatedRootURIs].map(runChecksForRoot));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impact: runChecks won't run when triggered from a file that you can't find a root for.


return;
Expand Down
Loading
0