Gracefully handle files that are not from a theme (#985)

Make findThemeRootURI return null when it can't find a root

Make everywhere handle null when no root is found

Should fix "preloading" of theme when looking at `$HOME/Library/Application Support/Code/User/settings.json`

Fixes #973

Author: charlespwd

.changeset/violet-cycles-yawn.md

@@ -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

packages/theme-check-common/src/checks/valid-visible-if/visible-if-utils.ts

@@ -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 };
@@ -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);
+    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) {

packages/theme-check-common/src/find-root.ts

@@ -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'))),
@@ -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;
@@ -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);

packages/theme-check-common/src/index.ts

@@ -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({

packages/theme-check-common/src/test/MockFileSystem.ts

@@ -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;
@@ -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') {
@@ -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;
   }
 
   private rootRelative(uri: string) {

packages/theme-check-common/src/test/test-helper.ts

@@ -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;

packages/theme-check-node/src/find-root.spec.ts

@@ -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': '{}',

packages/theme-language-server-common/src/diagnostics/runChecks.spec.ts

@@ -1,4 +1,5 @@
 import {
+  AbstractFileSystem,
   allChecks,
   LiquidCheckDefinition,
   path,
@@ -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 = {
@@ -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 () => [],
@@ -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: [
         {
@@ -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: [],
     });
@@ -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: [
@@ -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,
@@ -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({
@@ -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 () => [],
@@ -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);

packages/theme-language-server-common/src/diagnostics/runChecks.ts

@@ -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));
 
     return;