From 72a46babf12fa7c60bcfca7adf403b682baba75b Mon Sep 17 00:00:00 2001 From: James Meng Date: Fri, 5 Apr 2024 10:57:06 -0700 Subject: [PATCH 1/2] Improve Glob Pattern subdirectory mismatch error handling --- .changeset/spotty-badgers-attack.md | 5 +++ .../src/cli/utilities/asset-ignore.test.ts | 31 ++++++++++++++++++- .../theme/src/cli/utilities/asset-ignore.ts | 14 +++++++-- 3 files changed, 46 insertions(+), 4 deletions(-) create mode 100644 .changeset/spotty-badgers-attack.md diff --git a/.changeset/spotty-badgers-attack.md b/.changeset/spotty-badgers-attack.md new file mode 100644 index 00000000000..82427d97e59 --- /dev/null +++ b/.changeset/spotty-badgers-attack.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme': patch +--- + +Improve Glob Pattern subdirectory mismatch error handling diff --git a/packages/theme/src/cli/utilities/asset-ignore.test.ts b/packages/theme/src/cli/utilities/asset-ignore.test.ts index 58b08e4565b..2be237fecae 100644 --- a/packages/theme/src/cli/utilities/asset-ignore.test.ts +++ b/packages/theme/src/cli/utilities/asset-ignore.test.ts @@ -1,5 +1,6 @@ import {applyIgnoreFilters} from './asset-ignore.js' -import {ReadOptions, fileExists, readFile} from '@shopify/cli-kit/node/fs' +import {ReadOptions, fileExists, matchGlob, readFile} from '@shopify/cli-kit/node/fs' +import {outputWarn} from '@shopify/cli-kit/node/output' import {joinPath} from '@shopify/cli-kit/node/path' import {test, describe, beforeEach, vi, expect} from 'vitest' @@ -14,6 +15,7 @@ vi.mock('@shopify/cli-kit/node/fs', async () => { }) vi.mock('@shopify/cli-kit/node/path') +vi.mock('@shopify/cli-kit/node/output') describe('applyIgnoreFilters', () => { const checksums = [ @@ -129,4 +131,31 @@ describe('applyIgnoreFilters', () => { {key: 'templates/customers/account.json', checksum: '7777777777777777777777777777777'}, ]) }) + + test(`does not output warnings when there are no glob pattern modifications required for subdirectories`, async () => { + // Given + const options = {only: ['templates/*.json']} + + const filteredChecksums = checksums.filter(({key}) => !matchGlob(key, 'templates/**/*.json')) + + // When + await applyIgnoreFilters(filteredChecksums, themeFileSystem, options) + + // Then + expect(vi.mocked(outputWarn)).not.toHaveBeenCalled() + }) + + test(`outputs warnings when there are glob pattern modifications required for subdirectories`, async () => { + // Given + const options = {only: ['templates/*.json']} + const numFilesInSubDirectory = checksums.filter( + ({key}) => matchGlob(key, 'templates/**/*.json') && !matchGlob(key, 'templates/*.json'), + ).length + + // When + await applyIgnoreFilters(checksums, themeFileSystem, options) + + // Then + expect(vi.mocked(outputWarn)).toHaveBeenCalledTimes(numFilesInSubDirectory) + }) }) diff --git a/packages/theme/src/cli/utilities/asset-ignore.ts b/packages/theme/src/cli/utilities/asset-ignore.ts index 581a1d94a1a..cfe82596045 100644 --- a/packages/theme/src/cli/utilities/asset-ignore.ts +++ b/packages/theme/src/cli/utilities/asset-ignore.ts @@ -56,9 +56,17 @@ function matchGlob(key: string, pattern: string) { // When the the standard match fails and the pattern includes '/*.', we // replace '/*.' with '/**/*.' to emulate Shopify CLI 2.x behavior, as it was // based on 'File.fnmatch'. - if (pattern.includes('/*.')) { - outputWarn(`The pattern ${pattern} is unsafe, please use a valid regex or glob.`) - return originalMatchGlob(key, pattern.replace('/*.', '/**/*.')) + if (pattern.includes('/*.') && !pattern.includes('/**/*.')) { + const newPatternMatch = originalMatchGlob(key, pattern.replace('/*.', '/**/*.')) + if (newPatternMatch) { + outputWarn( + `Warning: File ${key} does not match the pattern '${pattern}'. To maintain backwards compatibility, we have modified your pattern to ${pattern.replace( + '/*.', + '/**/*.', + )} to explicitly include subdirectories.`, + ) + } + return newPatternMatch } return false From e1f01b0a414efc37e0a93c6321b4d6171f13a0f9 Mon Sep 17 00:00:00 2001 From: James Meng Date: Mon, 8 Apr 2024 18:13:47 -0700 Subject: [PATCH 2/2] Modify glob pattern subdirectory warning to only render once per execution --- .../src/cli/utilities/asset-ignore.test.ts | 28 ++++++++------- .../theme/src/cli/utilities/asset-ignore.ts | 36 +++++++++++++------ 2 files changed, 41 insertions(+), 23 deletions(-) diff --git a/packages/theme/src/cli/utilities/asset-ignore.test.ts b/packages/theme/src/cli/utilities/asset-ignore.test.ts index 2be237fecae..3ec68a08928 100644 --- a/packages/theme/src/cli/utilities/asset-ignore.test.ts +++ b/packages/theme/src/cli/utilities/asset-ignore.test.ts @@ -1,5 +1,5 @@ import {applyIgnoreFilters} from './asset-ignore.js' -import {ReadOptions, fileExists, matchGlob, readFile} from '@shopify/cli-kit/node/fs' +import {ReadOptions, fileExists, readFile} from '@shopify/cli-kit/node/fs' import {outputWarn} from '@shopify/cli-kit/node/output' import {joinPath} from '@shopify/cli-kit/node/path' import {test, describe, beforeEach, vi, expect} from 'vitest' @@ -132,15 +132,11 @@ describe('applyIgnoreFilters', () => { ]) }) - test(`does not output warnings when there are no glob pattern modifications required for subdirectories`, async () => { + test(`only outputs glob pattern subdirectory warnings for the templates folder`, async () => { // Given - const options = {only: ['templates/*.json']} - - const filteredChecksums = checksums.filter(({key}) => !matchGlob(key, 'templates/**/*.json')) - + const options = {only: ['assets/*.json', 'config/*.json', 'sections/*.json']} // When - await applyIgnoreFilters(filteredChecksums, themeFileSystem, options) - + await applyIgnoreFilters(checksums, themeFileSystem, options) // Then expect(vi.mocked(outputWarn)).not.toHaveBeenCalled() }) @@ -148,14 +144,22 @@ describe('applyIgnoreFilters', () => { test(`outputs warnings when there are glob pattern modifications required for subdirectories`, async () => { // Given const options = {only: ['templates/*.json']} - const numFilesInSubDirectory = checksums.filter( - ({key}) => matchGlob(key, 'templates/**/*.json') && !matchGlob(key, 'templates/*.json'), - ).length // When await applyIgnoreFilters(checksums, themeFileSystem, options) // Then - expect(vi.mocked(outputWarn)).toHaveBeenCalledTimes(numFilesInSubDirectory) + expect(vi.mocked(outputWarn)).toHaveBeenCalledTimes(1) + }) + + test('only outputs a single warning for duplicated glob patterns', async () => { + // Given + const options = {only: ['templates/*.json'], ignore: ['templates/*.json']} + + // When + await applyIgnoreFilters(checksums, themeFileSystem, options) + + // Then + expect(vi.mocked(outputWarn)).toHaveBeenCalledTimes(1) }) }) diff --git a/packages/theme/src/cli/utilities/asset-ignore.ts b/packages/theme/src/cli/utilities/asset-ignore.ts index cfe82596045..cdf335a6da5 100644 --- a/packages/theme/src/cli/utilities/asset-ignore.ts +++ b/packages/theme/src/cli/utilities/asset-ignore.ts @@ -12,15 +12,20 @@ export async function applyIgnoreFilters( ) { const shopifyIgnore = await shopifyIgnoredPatterns(themeFileSystem) + const ignoreOptions = options.ignore ?? [] + const onlyOptions = options.only ?? [] + + raiseWarningForNonExplicitGlobPatterns([...shopifyIgnore, ...ignoreOptions, ...onlyOptions]) + return themeChecksums .filter(filterBy(shopifyIgnore, '.shopifyignore')) - .filter(filterBy(options.ignore, '--ignore')) - .filter(filterBy(options.only, '--only', true)) + .filter(filterBy(ignoreOptions, '--ignore')) + .filter(filterBy(onlyOptions, '--only', true)) } -function filterBy(patterns: string[] | undefined, type: string, invertMatch = false) { +function filterBy(patterns: string[], type: string, invertMatch = false) { return ({key}: Checksum) => { - if (!patterns) return true + if (patterns.length === 0) return true const match = patterns.some((pattern) => matchGlob(key, pattern) || regexMatch(key, pattern)) const shouldIgnore = invertMatch ? !match : match @@ -56,20 +61,29 @@ function matchGlob(key: string, pattern: string) { // When the the standard match fails and the pattern includes '/*.', we // replace '/*.' with '/**/*.' to emulate Shopify CLI 2.x behavior, as it was // based on 'File.fnmatch'. - if (pattern.includes('/*.') && !pattern.includes('/**/*.')) { - const newPatternMatch = originalMatchGlob(key, pattern.replace('/*.', '/**/*.')) - if (newPatternMatch) { + if (shouldReplaceGlobPattern(pattern)) { + return originalMatchGlob(key, pattern.replace('/*.', '/**/*.')) + } + + return false +} + +function raiseWarningForNonExplicitGlobPatterns(patterns: string[]) { + const allPatterns = new Set(patterns) + allPatterns.forEach((pattern) => { + if (shouldReplaceGlobPattern(pattern)) { outputWarn( - `Warning: File ${key} does not match the pattern '${pattern}'. To maintain backwards compatibility, we have modified your pattern to ${pattern.replace( + `Warning: The pattern '${pattern}' does not include subdirectories. To maintain backwards compatibility, we have modified your pattern to ${pattern.replace( '/*.', '/**/*.', )} to explicitly include subdirectories.`, ) } - return newPatternMatch - } + }) +} - return false +function shouldReplaceGlobPattern(pattern: string): boolean { + return pattern.includes('/*.') && !pattern.includes('/**/*.') && pattern.includes('templates/') } function regexMatch(key: string, pattern: string) {