From 67be4a7142c6cf58330f401f18e02de04262baf3 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Mon, 11 Mar 2024 08:17:26 -0400 Subject: [PATCH] Add unit tests + changeset --- .changeset/curly-bikes-confess.md | 5 +++ packages/theme-check-common/src/path.ts | 2 - .../theme-check-common/src/to-source-code.ts | 10 ++++- packages/theme-check-common/src/types.ts | 14 ++++-- .../theme-check-node/src/find-root.spec.ts | 44 +------------------ packages/theme-check-node/src/index.spec.ts | 41 +++++++++++++++++ packages/theme-check-node/src/index.ts | 11 ++--- .../theme-check-node/src/test/test-helpers.ts | 42 +++++++++++++++++- 8 files changed, 114 insertions(+), 55 deletions(-) create mode 100644 .changeset/curly-bikes-confess.md create mode 100644 packages/theme-check-node/src/index.spec.ts diff --git a/.changeset/curly-bikes-confess.md b/.changeset/curly-bikes-confess.md new file mode 100644 index 000000000..9f35740b7 --- /dev/null +++ b/.changeset/curly-bikes-confess.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme-check-node': patch +--- + +Fix theme file glob on Windows (CLI issue) diff --git a/packages/theme-check-common/src/path.ts b/packages/theme-check-common/src/path.ts index 0dbc7c5f6..08ef6b8e7 100644 --- a/packages/theme-check-common/src/path.ts +++ b/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('/'); } diff --git a/packages/theme-check-common/src/to-source-code.ts b/packages/theme-check-common/src/to-source-code.ts index c4106df1d..4b77ed5cf 100644 --- a/packages/theme-check-common/src/to-source-code.ts +++ b/packages/theme-check-common/src/to-source-code.ts @@ -40,7 +40,7 @@ export function toSourceCode( if (isLiquid) { return { - absolutePath, + absolutePath: normalize(absolutePath), source, type: SourceCodeType.LiquidHtml, ast: parseLiquid(source), @@ -48,7 +48,7 @@ export function toSourceCode( }; } else { return { - absolutePath, + absolutePath: normalize(absolutePath), source, type: SourceCodeType.JSON, ast: parseJSON(source), @@ -56,3 +56,9 @@ export function toSourceCode( }; } } + +type MaybeWindowsPath = string; + +function normalize(path: MaybeWindowsPath): string { + return path.replace(/\\/g, '/'); +} diff --git a/packages/theme-check-common/src/types.ts b/packages/theme-check-common/src/types.ts index a73679c73..6b0314137 100644 --- a/packages/theme-check-common/src/types.ts +++ b/packages/theme-check-common/src/types.ts @@ -25,11 +25,16 @@ export type Theme = SourceCode[]; export type SourceCode = 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 = { diff --git a/packages/theme-check-node/src/find-root.spec.ts b/packages/theme-check-node/src/find-root.spec.ts index 8cbb0cac8..c7c0e1ee2 100644 --- a/packages/theme-check-node/src/find-root.spec.ts +++ b/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; -} - -async function makeTempWorkspace(structure: Tree): Promise { - 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 { - const promises: Promise[] = []; - 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); - } -} diff --git a/packages/theme-check-node/src/index.spec.ts b/packages/theme-check-node/src/index.spec.ts new file mode 100644 index 000000000..a547dcdc3 --- /dev/null +++ b/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( + path.normalize(workspace.path('locales/en.default.json')), + ); + }); +}); diff --git a/packages/theme-check-node/src/index.ts b/packages/theme-check-node/src/index.ts index 3a540d353..05daababd 100644 --- a/packages/theme-check-node/src/index.ts +++ b/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,15 +103,15 @@ async function getThemeAndConfig( } export async function getTheme(config: Config): Promise { - - // On windows machines - the separator provided by path.join is '\' + // 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 normalized_path = path.normalize(path.join(config.root, '**/*.{liquid,json}')).replace(/\\/g, '/'); - - const paths = await asyncGlob(normalized_path).then((result) => + 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)), ); diff --git a/packages/theme-check-node/src/test/test-helpers.ts b/packages/theme-check-node/src/test/test-helpers.ts index 894744903..07ccb04e2 100644 --- a/packages/theme-check-node/src/test/test-helpers.ts +++ b/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; +} + +export async function makeTempWorkspace(structure: Tree): Promise { + 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 { + const promises: Promise[] = []; + 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); + } +}