From 7b80ebba7d4e87d27c9aaa90ce333ffa5d4b3b47 Mon Sep 17 00:00:00 2001 From: Miguel Montalvo Date: Sun, 13 Oct 2024 15:29:14 -0600 Subject: [PATCH 1/7] Check if the `pattern` is a regex and create a RegExp instance with it --- .../src/cli/utilities/asset-ignore.test.ts | 2 +- .../theme/src/cli/utilities/asset-ignore.ts | 23 ++++++++++++------- .../cli/utilities/theme-downloader.test.ts | 2 +- 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/packages/theme/src/cli/utilities/asset-ignore.test.ts b/packages/theme/src/cli/utilities/asset-ignore.test.ts index b9aa35d12f..04ac0f3910 100644 --- a/packages/theme/src/cli/utilities/asset-ignore.test.ts +++ b/packages/theme/src/cli/utilities/asset-ignore.test.ts @@ -89,7 +89,7 @@ describe('asset-ignore', () => { 'sections/*', 'templates/*', 'config/*_data.json', - '.*settings_schema.json', + '/settings_schema/', ], } diff --git a/packages/theme/src/cli/utilities/asset-ignore.ts b/packages/theme/src/cli/utilities/asset-ignore.ts index eba1239ac3..6f8426cd11 100644 --- a/packages/theme/src/cli/utilities/asset-ignore.ts +++ b/packages/theme/src/cli/utilities/asset-ignore.ts @@ -24,7 +24,9 @@ function filterBy(patterns: string[], type: string, invertMatch = false) { return ({key}: {key: string}) => { if (patterns.length === 0) return true - const match = patterns.some((pattern) => matchGlob(key, pattern) || regexMatch(key, pattern)) + const match = patterns.some( + (pattern) => matchGlob(key, pattern) || (isRegex(pattern) && regexMatch(key, asRegex(pattern))), + ) const shouldIgnore = invertMatch ? !match : match if (shouldIgnore) { @@ -88,11 +90,16 @@ function shouldReplaceGlobPattern(pattern: string): boolean { return pattern.includes('/*.') && !pattern.includes('/**/*.') && pattern.includes('templates/') } -function regexMatch(key: string, pattern: string) { - try { - return key.match(pattern) - // eslint-disable-next-line no-catch-all/no-catch-all - } catch { - return false - } +function regexMatch(key: string, regex: RegExp) { + return regex.test(key) +} + +// https://github.com/Shopify/cli/blob/2ddbd3eee70c50814c5527d6d3eeb7ca601de5f8/packages/cli-kit/assets/cli-ruby/lib/shopify_cli/theme/filter/path_matcher.rb#L17 +function isRegex(pattern: string) { + return pattern.startsWith('/') && pattern.endsWith('/') +} + +// https://github.com/Shopify/cli/blob/2ddbd3eee70c50814c5527d6d3eeb7ca601de5f8/packages/cli-kit/assets/cli-ruby/lib/shopify_cli/theme/filter/path_matcher.rb#L21 +function asRegex(pattern: string) { + return new RegExp(pattern.slice(1, -1)) } diff --git a/packages/theme/src/cli/utilities/theme-downloader.test.ts b/packages/theme/src/cli/utilities/theme-downloader.test.ts index e35c9d5118..055c3785e1 100644 --- a/packages/theme/src/cli/utilities/theme-downloader.test.ts +++ b/packages/theme/src/cli/utilities/theme-downloader.test.ts @@ -71,7 +71,7 @@ describe('theme-downloader', () => { test('downloads missing remote files', async () => { // Given - const downloadOptions = {nodelete: false, only: ['release'], ignore: ['release/ignoreme']} + const downloadOptions = {nodelete: false, only: ['/release/'], ignore: ['release/ignoreme']} const fileToDownload = {key: 'release/downloadme', checksum: '1'} const files = new Map([ ['release/alreadyexists', {checksum: '2', value: 'content', key: 'release/alreadyexists'}], From e20fb5ffc2f6245a7a11ce445da6477713b0f93f Mon Sep 17 00:00:00 2001 From: Miguel Montalvo Date: Sun, 13 Oct 2024 16:46:51 -0600 Subject: [PATCH 2/7] Change templates pattern to `'templates/*'` so that 'templates/*' and 'templates/*.json' get replaced --- packages/theme/src/cli/utilities/asset-ignore.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/theme/src/cli/utilities/asset-ignore.ts b/packages/theme/src/cli/utilities/asset-ignore.ts index 6f8426cd11..bdb6fac182 100644 --- a/packages/theme/src/cli/utilities/asset-ignore.ts +++ b/packages/theme/src/cli/utilities/asset-ignore.ts @@ -63,10 +63,10 @@ function matchGlob(key: string, pattern: string) { if (result) return true // When the the standard match fails and the pattern includes '/*.', we - // replace '/*.' with '/**/*.' to emulate Shopify CLI 2.x behavior, as it was + // replace '/*' with '/**/*' to emulate Shopify CLI 2.x behavior, as it was // based on 'File.fnmatch'. if (shouldReplaceGlobPattern(pattern)) { - return originalMatchGlob(key, pattern.replace('/*.', '/**/*.'), matchOpts) + return originalMatchGlob(key, pattern.replace('/*', '/**/*'), matchOpts) } return false @@ -78,8 +78,8 @@ export function raiseWarningForNonExplicitGlobPatterns(patterns: string[]) { 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.`, ) } @@ -87,7 +87,7 @@ export function raiseWarningForNonExplicitGlobPatterns(patterns: string[]) { } function shouldReplaceGlobPattern(pattern: string): boolean { - return pattern.includes('/*.') && !pattern.includes('/**/*.') && pattern.includes('templates/') + return pattern.includes('/*') && !pattern.includes('/**/*') && pattern.includes('templates/') } function regexMatch(key: string, regex: RegExp) { From 90f92c253f501dbdfc97aace70bcacdea75b7f9d Mon Sep 17 00:00:00 2001 From: Miguel Montalvo Date: Sun, 13 Oct 2024 19:15:40 -0600 Subject: [PATCH 3/7] Refactor `matchGlob` and `shouldReplaceGlobPattern` so they don't match/replace a minimatch pattern like 'templates/**/*' --- packages/theme/src/cli/utilities/asset-ignore.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/theme/src/cli/utilities/asset-ignore.ts b/packages/theme/src/cli/utilities/asset-ignore.ts index bdb6fac182..45c9b425a1 100644 --- a/packages/theme/src/cli/utilities/asset-ignore.ts +++ b/packages/theme/src/cli/utilities/asset-ignore.ts @@ -3,6 +3,7 @@ import {outputDebug, outputWarn} from '@shopify/cli-kit/node/output' import {joinPath} from '@shopify/cli-kit/node/path' const SHOPIFY_IGNORE = '.shopifyignore' +const templatesRegex = /templates\/\*(\.(json|liquid))?$/ export function applyIgnoreFilters( files: T[], @@ -66,7 +67,7 @@ function matchGlob(key: string, pattern: string) { // replace '/*' with '/**/*' to emulate Shopify CLI 2.x behavior, as it was // based on 'File.fnmatch'. if (shouldReplaceGlobPattern(pattern)) { - return originalMatchGlob(key, pattern.replace('/*', '/**/*'), matchOpts) + return originalMatchGlob(key, pattern.replace(templatesRegex, 'templates/**/*$1'), matchOpts) } return false @@ -87,7 +88,7 @@ export function raiseWarningForNonExplicitGlobPatterns(patterns: string[]) { } function shouldReplaceGlobPattern(pattern: string): boolean { - return pattern.includes('/*') && !pattern.includes('/**/*') && pattern.includes('templates/') + return templatesRegex.test(pattern) } function regexMatch(key: string, regex: RegExp) { From 39a96fc53c6041c36874a27766464fb39582af31 Mon Sep 17 00:00:00 2001 From: Miguel Montalvo Date: Sun, 13 Oct 2024 20:45:34 -0600 Subject: [PATCH 4/7] Enable globstar `**` in minimatch patterns so developers can use it to avoid backward compatibility warnings on the 'template/*' pattern. --- packages/theme/src/cli/utilities/asset-ignore.test.ts | 3 ++- packages/theme/src/cli/utilities/asset-ignore.ts | 10 +++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/theme/src/cli/utilities/asset-ignore.test.ts b/packages/theme/src/cli/utilities/asset-ignore.test.ts index 04ac0f3910..f9d6d75839 100644 --- a/packages/theme/src/cli/utilities/asset-ignore.test.ts +++ b/packages/theme/src/cli/utilities/asset-ignore.test.ts @@ -162,7 +162,7 @@ describe('asset-ignore', () => { ]) }) - test(`matching is backward compatible with Shopify CLI 2 with the templates/**/*.json pattern`, async () => { + test(`matching supports minimatch patterns like templates/**/*.json`, async () => { // Given const options = {only: ['templates/**/*.json']} @@ -171,6 +171,7 @@ describe('asset-ignore', () => { // Then expect(actualChecksums).toEqual([ + {key: 'templates/404.json', checksum: '6666666666666666666666666666666'}, {key: 'templates/customers/account.json', checksum: '7777777777777777777777777777777'}, ]) }) diff --git a/packages/theme/src/cli/utilities/asset-ignore.ts b/packages/theme/src/cli/utilities/asset-ignore.ts index 45c9b425a1..5a7b742630 100644 --- a/packages/theme/src/cli/utilities/asset-ignore.ts +++ b/packages/theme/src/cli/utilities/asset-ignore.ts @@ -56,7 +56,7 @@ export async function getPatternsFromShopifyIgnore(root: string) { function matchGlob(key: string, pattern: string) { const matchOpts = { matchBase: true, - noglobstar: true, + noglobstar: false, } const result = originalMatchGlob(key, pattern, matchOpts) @@ -78,10 +78,10 @@ export function raiseWarningForNonExplicitGlobPatterns(patterns: string[]) { 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.`, + `Warning: The pattern '${pattern}' excludes subdirectories. To maintain backwards compatibility, we've changed it to '${pattern.replace( + templatesRegex, + 'templates/**/*$1', + )}'. Use '${pattern.replace(templatesRegex, 'templates/**/*$1')}' to avoid this warning.`, ) } }) From 89250c3016a8fb13481fa368f8eaeeaadbb89e23 Mon Sep 17 00:00:00 2001 From: Miguel Montalvo Date: Sun, 13 Oct 2024 21:50:26 -0600 Subject: [PATCH 5/7] Add release note --- .changeset/wicked-apricots-shake.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/wicked-apricots-shake.md diff --git a/.changeset/wicked-apricots-shake.md b/.changeset/wicked-apricots-shake.md new file mode 100644 index 0000000000..07e62dd36f --- /dev/null +++ b/.changeset/wicked-apricots-shake.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme': patch +--- + +Fix ignore patterns From 5491866be82612f58f6e748f7e2660db1458d44f Mon Sep 17 00:00:00 2001 From: Miguel Montalvo Date: Tue, 15 Oct 2024 06:27:22 -0600 Subject: [PATCH 6/7] Revert "Enable globstar `**` in minimatch patterns" This reverts commit 39a96fc53c6041c36874a27766464fb39582af31. --- packages/theme/src/cli/utilities/asset-ignore.test.ts | 3 +-- packages/theme/src/cli/utilities/asset-ignore.ts | 10 +++++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/packages/theme/src/cli/utilities/asset-ignore.test.ts b/packages/theme/src/cli/utilities/asset-ignore.test.ts index f9d6d75839..04ac0f3910 100644 --- a/packages/theme/src/cli/utilities/asset-ignore.test.ts +++ b/packages/theme/src/cli/utilities/asset-ignore.test.ts @@ -162,7 +162,7 @@ describe('asset-ignore', () => { ]) }) - test(`matching supports minimatch patterns like templates/**/*.json`, async () => { + test(`matching is backward compatible with Shopify CLI 2 with the templates/**/*.json pattern`, async () => { // Given const options = {only: ['templates/**/*.json']} @@ -171,7 +171,6 @@ describe('asset-ignore', () => { // Then expect(actualChecksums).toEqual([ - {key: 'templates/404.json', checksum: '6666666666666666666666666666666'}, {key: 'templates/customers/account.json', checksum: '7777777777777777777777777777777'}, ]) }) diff --git a/packages/theme/src/cli/utilities/asset-ignore.ts b/packages/theme/src/cli/utilities/asset-ignore.ts index 5a7b742630..45c9b425a1 100644 --- a/packages/theme/src/cli/utilities/asset-ignore.ts +++ b/packages/theme/src/cli/utilities/asset-ignore.ts @@ -56,7 +56,7 @@ export async function getPatternsFromShopifyIgnore(root: string) { function matchGlob(key: string, pattern: string) { const matchOpts = { matchBase: true, - noglobstar: false, + noglobstar: true, } const result = originalMatchGlob(key, pattern, matchOpts) @@ -78,10 +78,10 @@ export function raiseWarningForNonExplicitGlobPatterns(patterns: string[]) { allPatterns.forEach((pattern) => { if (shouldReplaceGlobPattern(pattern)) { outputWarn( - `Warning: The pattern '${pattern}' excludes subdirectories. To maintain backwards compatibility, we've changed it to '${pattern.replace( - templatesRegex, - 'templates/**/*$1', - )}'. Use '${pattern.replace(templatesRegex, 'templates/**/*$1')}' to avoid this warning.`, + `Warning: The pattern '${pattern}' does not include subdirectories. To maintain backwards compatibility, we have modified your pattern to ${pattern.replace( + '/*', + '/**/*', + )} to explicitly include subdirectories.`, ) } }) From cd357970f61ed190cba6543daa73cf5f743ed7e2 Mon Sep 17 00:00:00 2001 From: Miguel Montalvo Date: Tue, 15 Oct 2024 06:42:40 -0600 Subject: [PATCH 7/7] Revert "Change templates pattern to `'templates/*'`" This reverts commit e20fb5ffc2f6245a7a11ce445da6477713b0f93f. --- packages/theme/src/cli/utilities/asset-ignore.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/theme/src/cli/utilities/asset-ignore.ts b/packages/theme/src/cli/utilities/asset-ignore.ts index 45c9b425a1..37e42c5dff 100644 --- a/packages/theme/src/cli/utilities/asset-ignore.ts +++ b/packages/theme/src/cli/utilities/asset-ignore.ts @@ -64,7 +64,7 @@ function matchGlob(key: string, pattern: string) { if (result) return true // When the the standard match fails and the pattern includes '/*.', we - // replace '/*' with '/**/*' to emulate Shopify CLI 2.x behavior, as it was + // replace '/*.' with '/**/*.' to emulate Shopify CLI 2.x behavior, as it was // based on 'File.fnmatch'. if (shouldReplaceGlobPattern(pattern)) { return originalMatchGlob(key, pattern.replace(templatesRegex, 'templates/**/*$1'), matchOpts) @@ -79,8 +79,8 @@ export function raiseWarningForNonExplicitGlobPatterns(patterns: string[]) { if (shouldReplaceGlobPattern(pattern)) { outputWarn( `Warning: The pattern '${pattern}' does not include subdirectories. To maintain backwards compatibility, we have modified your pattern to ${pattern.replace( - '/*', - '/**/*', + templatesRegex, + 'templates/**/*$1', )} to explicitly include subdirectories.`, ) }