Fix theme files not being found on Windows for theme check

.changeset/curly-bikes-confess.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme-check-node': patch
+---
+
+Fix theme file glob on Windows (CLI issue)

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

@@ -1,12 +1,10 @@
 import { RelativePath, AbsolutePath } from './types';
 
 export function relative(absolutePath: AbsolutePath, root: AbsolutePath): RelativePath {
-  // TODO (#15): do the right thing for windows and shit.
   return absolutePath.replace(root, '').replace(/^\//, '');
 }
 
 export function join(...paths: string[]): string {
-  // TODO (#15): do the right thing, collapse slashes and shit.
   return paths.map(removeTrailingSlash).join('/');
 }
 

packages/theme-check-common/src/to-source-code.ts

@@ -40,19 +40,25 @@ export function toSourceCode(
 
   if (isLiquid) {
     return {
-      absolutePath,
+      absolutePath: normalize(absolutePath),
       source,
       type: SourceCodeType.LiquidHtml,
       ast: parseLiquid(source),
       version,
     };
   } else {
     return {
-      absolutePath,
+      absolutePath: normalize(absolutePath),
       source,
       type: SourceCodeType.JSON,
       ast: parseJSON(source),
       version,
     };
   }
 }
+
+type MaybeWindowsPath = string;
+
+function normalize(path: MaybeWindowsPath): string {
+  return path.replace(/\\/g, '/');
+}

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

@@ -25,11 +25,16 @@ export type Theme = SourceCode<SourceCodeType>[];
 
 export type SourceCode<T = SourceCodeType> = T extends SourceCodeType
   ? {
-      absolutePath: string; // /path/to/snippet/foo.liquid
+      /** A normalized absolute path to the file. Assumes forwards slashes. */
+      absolutePath: string;
+      /** The type is used as a discriminant for type narrowing */
+      type: T;
+      /** The version is used by the Language Server to make sure the Client and Server are in sync */
       version?: number;
+      /** The contents of the file */
       source: string;
-      type: T; // Liquid | LiquidHtml | JSON
-      ast: AST[T] | Error; // LiquidAST | LiquidHtmlAST | JSON object | null when unparseable
+      /** The AST representation of the file, or an Error instance when the file is unparseable */
+      ast: AST[T] | Error;
     }
   : never;
 
@@ -74,7 +79,10 @@ export type NodeTypes = {
   }[T];
 };
 
+/** Assumes forward slashes for simplicity internally */
 export type AbsolutePath = string;
+
+/** Assumes forward slashes for simplicity internally */
 export type RelativePath = string;
 
 export type ChecksSettings = {

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

@@ -1,8 +1,7 @@
 import { describe, it, expect, beforeEach, afterEach, beforeAll, afterAll } from 'vitest';
 import { findRoot } from './find-root';
-import * as path from 'node:path';
-import * as fs from 'node:fs/promises';
-import * as mktemp from 'mktemp';
+import { Workspace } from './test/test-helpers';
+import { makeTempWorkspace } from './test/test-helpers';
 
 const theme = {
   locales: {
@@ -120,42 +119,3 @@ describe('Unit: findRoot', () => {
     expect(root).toBe(workspace.path('taeRootThemeCheckYML'));
   });
 });
-
-type Tree = {
-  [k in string]: Tree | string;
-};
-
-interface Workspace {
-  root: string;
-  path(relativePath: string): string;
-  clean(): Promise<any>;
-}
-
-async function makeTempWorkspace(structure: Tree): Promise<Workspace> {
-  const root = await mktemp.createDir(path.join(__dirname, '..', '.XXXXX'));
-  if (!root) throw new Error('Could not create temp dir for temp workspace');
-
-  await createFiles(structure, [root]);
-
-  return {
-    root,
-    path: (relativePath) => path.join(root, ...relativePath.split('/')),
-    clean: async () => fs.rm(root, { recursive: true, force: true }),
-  };
-
-  function createFiles(tree: Tree, ancestors: string[]): Promise<any> {
-    const promises: Promise<any>[] = [];
-    for (const [pathEl, value] of Object.entries(tree)) {
-      if (typeof value === 'string') {
-        promises.push(fs.writeFile(path.join(...ancestors, pathEl), value, 'utf8'));
-      } else {
-        promises.push(
-          fs
-            .mkdir(path.join(...ancestors, pathEl))
-            .then(() => createFiles(value, ancestors.concat(pathEl))),
-        );
-      }
-    }
-    return Promise.all(promises);
-  }
-}

packages/theme-check-node/src/index.spec.ts

@@ -0,0 +1,41 @@
+import { describe, it, expect, assert, beforeEach, afterEach } from 'vitest';
+import * as path from 'node:path';
+import { Config, SourceCodeType, getTheme } from './index';
+import { Workspace, makeTempWorkspace } from './test/test-helpers';
+
+describe('Unit: getTheme', () => {
+  let workspace: Workspace;
+
+  beforeEach(async () => {
+    workspace = await makeTempWorkspace({
+      locales: {
+        'en.default.json': '{}',
+      },
+      snippets: {
+        'header.liquid': '',
+      },
+    });
+  });
+
+  afterEach(async () => {
+    await workspace.clean();
+  });
+
+  it('should correctly get theme on all platforms', async () => {
+    const config: Config = {
+      checks: [],
+      root: workspace.root,
+      settings: {},
+    };
+
+    const theme = await getTheme(config);
+    expect(theme).to.have.lengthOf(2);
+    const jsonFile = theme.find((sc) => sc.type === SourceCodeType.JSON);
+    assert(jsonFile);
+
+    // internally we expect the path to be normalized
+    expect(jsonFile.absolutePath).to.equal(
+      workspace.path('locales/en.default.json').replace(/\\/g, '/'),
+    );
+  });
+});

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

@@ -75,6 +75,7 @@ export async function themeCheckRun(root: string, configPath?: string): Promise<
       if (!defaultTranslationsFile) {
         return defaultLocale;
       }
+      // assumes the path is normalized and '/' are used as separators
       const defaultTranslationsFileLocale = defaultTranslationsFile.absolutePath.match(
         /locales\/(.*)\.default\.json$/,
       )?.[1];
@@ -102,7 +103,15 @@ async function getThemeAndConfig(
 }
 
 export async function getTheme(config: Config): Promise<Theme> {
-  const paths = await asyncGlob(path.join(config.root, '**/*.{liquid,json}')).then((result) =>
+  // On windows machines - the separator provided by path.join is '\'
+  // however the glob function fails silently since '\' is used to escape glob charater
+  // as mentioned in the documentation of node-glob
+
+  // the path is normalised and '\' are replaced with '/' and then passed to the glob function
+  const normalizedGlob = path
+    .normalize(path.join(config.root, '**/*.{liquid,json}'))
+    .replace(/\\/g, '/');
+  const paths = await asyncGlob(normalizedGlob).then((result) =>
     // Global ignored paths should not be part of the theme
     result.filter((filePath) => !isIgnored(filePath, config)),
   );

packages/theme-check-node/src/test/test-helpers.ts

@@ -1,6 +1,7 @@
+import * as mktemp from 'mktemp';
 import fs from 'node:fs/promises';
-import path from 'node:path';
 import os from 'node:os';
+import path from 'node:path';
 
 export async function makeTmpFolder() {
   const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'test-'));
@@ -61,3 +62,42 @@ export async function createMockNodeModule(
   await fs.writeFile(path.join(nodeModuleRoot, 'index.js'), moduleContent);
   return nodeModuleRoot;
 }
+
+export type Tree = {
+  [k in string]: Tree | string;
+};
+
+export interface Workspace {
+  root: string;
+  path(relativePath: string): string;
+  clean(): Promise<any>;
+}
+
+export async function makeTempWorkspace(structure: Tree): Promise<Workspace> {
+  const root = await mktemp.createDir(path.join(__dirname, '..', '.XXXXX'));
+  if (!root) throw new Error('Could not create temp dir for temp workspace');
+
+  await createFiles(structure, [root]);
+
+  return {
+    root,
+    path: (relativePath) => path.join(root, ...relativePath.split('/')),
+    clean: async () => fs.rm(root, { recursive: true, force: true }),
+  };
+
+  function createFiles(tree: Tree, ancestors: string[]): Promise<any> {
+    const promises: Promise<any>[] = [];
+    for (const [pathEl, value] of Object.entries(tree)) {
+      if (typeof value === 'string') {
+        promises.push(fs.writeFile(path.join(...ancestors, pathEl), value, 'utf8'));
+      } else {
+        promises.push(
+          fs
+            .mkdir(path.join(...ancestors, pathEl))
+            .then(() => createFiles(value, ancestors.concat(pathEl))),
+        );
+      }
+    }
+    return Promise.all(promises);
+  }
+}