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..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,6 @@ import {applyIgnoreFilters} from './asset-ignore.js' 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' @@ -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,35 @@ describe('applyIgnoreFilters', () => { {key: 'templates/customers/account.json', checksum: '7777777777777777777777777777777'}, ]) }) + + test(`only outputs glob pattern subdirectory warnings for the templates folder`, async () => { + // Given + const options = {only: ['assets/*.json', 'config/*.json', 'sections/*.json']} + // When + await applyIgnoreFilters(checksums, 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']} + + // When + await applyIgnoreFilters(checksums, themeFileSystem, options) + + // Then + 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 581a1d94a1a..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,14 +61,31 @@ 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.`) + 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: The pattern '${pattern}' does not include subdirectories. To maintain backwards compatibility, we have modified your pattern to ${pattern.replace( + '/*.', + '/**/*.', + )} to explicitly include subdirectories.`, + ) + } + }) +} + +function shouldReplaceGlobPattern(pattern: string): boolean { + return pattern.includes('/*.') && !pattern.includes('/**/*.') && pattern.includes('templates/') +} + function regexMatch(key: string, pattern: string) { try { return key.match(pattern)