From 8ca5366cc2e0bfcb15126e27db33e33e6ef9a320 Mon Sep 17 00:00:00 2001
From: James Meng <james.meng@shopify.com>
Date: Mon, 8 Apr 2024 18:13:47 -0700
Subject: [PATCH] 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..0b9a183fd49 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 ?? []
+
+  checkPatterns([...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 checkPatterns(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) {