Merge pull request #4732 from Shopify/fix-issue-4710

Fix bug related to using negated patterns in shopify ignore
Shopify · Oct 31, 2024 · 0f8840e · 0f8840e
2 parents 4a1b164 + 2720923
commit 0f8840e
Show file tree

Hide file tree

Showing 5 changed files with 147 additions and 4 deletions.
diff --git a/.changeset/fresh-crabs-kick.md b/.changeset/fresh-crabs-kick.md
@@ -0,0 +1,5 @@
+---
+'@shopify/cli': patch
+---
+
+Fix files ignored when using negate patterns
diff --git a/packages/theme/src/cli/utilities/asset-ignore.test.ts b/packages/theme/src/cli/utilities/asset-ignore.test.ts
@@ -174,4 +174,114 @@ describe('asset-ignore', () => {
       ])
     })
   })
+
+  describe('applyIgnoreFilters with negated patterns', () => {
+    test(`only negating a file does not produce side effects of other ignored files`, () => {
+      const options = {
+        ignoreFromFile: ['!assets/basic.css'],
+      }
+
+      const actualChecksums = applyIgnoreFilters(checksums, options)
+
+      expect(actualChecksums).toEqual([
+        {key: 'assets/basic.css', checksum: '00000000000000000000000000000000'},
+        {key: 'assets/complex.css', checksum: '11111111111111111111111111111111'},
+        {key: 'assets/image.png', checksum: '22222222222222222222222222222222'},
+        {key: 'config/settings_data.json', checksum: '33333333333333333333333333333333'},
+        {key: 'config/settings_schema.json', checksum: '44444444444444444444444444444444'},
+        {key: 'sections/announcement-bar.liquid', checksum: '55555555555555555555555555555555'},
+        {key: 'templates/404.json', checksum: '6666666666666666666666666666666'},
+        {key: 'templates/customers/account.json', checksum: '7777777777777777777777777777777'},
+      ])
+    })
+    test(`negating a specific file overrides ignoring one`, () => {
+      // Given
+      const options = {
+        ignoreFromFile: ['assets/*.css', '!assets/basic.css'],
+      }
+
+      // When
+      const actualChecksums = applyIgnoreFilters(checksums, options)
+
+      // Then
+      expect(actualChecksums).toEqual([
+        {key: 'assets/image.png', checksum: '22222222222222222222222222222222'},
+        {key: 'config/settings_data.json', checksum: '33333333333333333333333333333333'},
+        {key: 'config/settings_schema.json', checksum: '44444444444444444444444444444444'},
+        {key: 'sections/announcement-bar.liquid', checksum: '55555555555555555555555555555555'},
+        {key: 'templates/404.json', checksum: '6666666666666666666666666666666'},
+        {key: 'templates/customers/account.json', checksum: '7777777777777777777777777777777'},
+        {key: 'assets/basic.css', checksum: '00000000000000000000000000000000'},
+      ])
+    })
+
+    test('should not ignore files matching a negated pattern', () => {
+      const ignorePatterns = [
+        'assets/basic.css',
+        'sections/*.json',
+        'templates/*.json',
+        'templates/**/*.json',
+        'config/*.json',
+        '!config/*_schema.json',
+      ]
+
+      const actualChecksums = applyIgnoreFilters(checksums, {ignoreFromFile: ignorePatterns})
+
+      expect(actualChecksums).toEqual([
+        {key: 'assets/complex.css', checksum: '11111111111111111111111111111111'},
+        {key: 'assets/image.png', checksum: '22222222222222222222222222222222'},
+        {key: 'sections/announcement-bar.liquid', checksum: '55555555555555555555555555555555'},
+        {key: 'config/settings_schema.json', checksum: '44444444444444444444444444444444'},
+      ])
+    })
+  })
+  describe('applyIgnoreFilters with negated ignore and only options', () => {
+    test(`negating a specific file in ignore option overrides ignoring one`, () => {
+      const options = {
+        ignore: ['assets/*.css', '!assets/basic.css'],
+      }
+
+      const actualChecksums = applyIgnoreFilters(checksums, options)
+
+      expect(actualChecksums).toEqual([
+        {key: 'assets/image.png', checksum: '22222222222222222222222222222222'},
+        {key: 'config/settings_data.json', checksum: '33333333333333333333333333333333'},
+        {key: 'config/settings_schema.json', checksum: '44444444444444444444444444444444'},
+        {key: 'sections/announcement-bar.liquid', checksum: '55555555555555555555555555555555'},
+        {key: 'templates/404.json', checksum: '6666666666666666666666666666666'},
+        {key: 'templates/customers/account.json', checksum: '7777777777777777777777777777777'},
+        {key: 'assets/basic.css', checksum: '00000000000000000000000000000000'},
+      ])
+    })
+
+    test(`negating a specific file in only option properly overrides and ignores it`, () => {
+      const options = {
+        only: ['assets/*.css', '!assets/basic.css'],
+      }
+
+      const actualChecksums = applyIgnoreFilters(checksums, options)
+
+      expect(actualChecksums).toEqual([{key: 'assets/complex.css', checksum: '11111111111111111111111111111111'}])
+    })
+  })
+  describe('applyIgnoreFilters do not return duplicates', () => {
+    test(`should not return duplicates when negated patterns are used`, () => {
+      const options = {
+        ignoreFromFile: ['assets/*.css', '!assets/basic.css'],
+        ignore: ['!assets/basic.css'],
+      }
+
+      const actualChecksums = applyIgnoreFilters(checksums, options)
+
+      expect(actualChecksums).toEqual([
+        {key: 'assets/image.png', checksum: '22222222222222222222222222222222'},
+        {key: 'config/settings_data.json', checksum: '33333333333333333333333333333333'},
+        {key: 'config/settings_schema.json', checksum: '44444444444444444444444444444444'},
+        {key: 'sections/announcement-bar.liquid', checksum: '55555555555555555555555555555555'},
+        {key: 'templates/404.json', checksum: '6666666666666666666666666666666'},
+        {key: 'templates/customers/account.json', checksum: '7777777777777777777777777777777'},
+        {key: 'assets/basic.css', checksum: '00000000000000000000000000000000'},
+      ])
+    })
+  })
 })
diff --git a/packages/theme/src/cli/utilities/asset-ignore.ts b/packages/theme/src/cli/utilities/asset-ignore.ts
@@ -1,3 +1,4 @@
+import {uniqBy} from '@shopify/cli-kit/common/array'
 import {fileExists, readFile, matchGlob as originalMatchGlob} from '@shopify/cli-kit/node/fs'
 import {outputDebug} from '@shopify/cli-kit/node/output'
 import {joinPath} from '@shopify/cli-kit/node/path'
@@ -13,10 +14,26 @@ export function applyIgnoreFilters<T extends {key: string}>(
   const ignoreOptions = options.ignore ?? []
   const onlyOptions = options.only ?? []
 
-  return files
-    .filter(filterBy(shopifyIgnore, '.shopifyignore'))
-    .filter(filterBy(ignoreOptions, '--ignore'))
-    .filter(filterBy(onlyOptions, '--only', true))
+  const [normalShopifyPatterns = [], negatedShopifyPatterns = []] = filterRegexValues(shopifyIgnore)
+  const [normalIgnorePatterns = [], negatedIgnorePatterns = []] = filterRegexValues(ignoreOptions)
+  const [normalOnlyPatterns = [], negatedOnlyPatterns = []] = filterRegexValues(onlyOptions)
+
+  let filteredFiles = files.filter(filterBy(normalShopifyPatterns, '.shopifyignore'))
+  filteredFiles = filteredFiles.filter(filterBy(normalIgnorePatterns, '--ignore'))
+  filteredFiles = filteredFiles.filter(filterBy(normalOnlyPatterns, '--only', true))
+
+  if (negatedShopifyPatterns.length > 0) {
+    filteredFiles = filteredFiles.concat(files.filter(filterBy(negatedShopifyPatterns, '.shopifyignore', true)))
+  }
+  if (negatedIgnorePatterns.length > 0) {
+    filteredFiles = filteredFiles.concat(files.filter(filterBy(negatedIgnorePatterns, '--ignore', true)))
+  }
+  if (negatedOnlyPatterns.length > 0) {
+    filteredFiles = filteredFiles.filter(filterBy(negatedOnlyPatterns, '--only'))
+  }
+
+  const uniqueFiles = uniqBy(filteredFiles, (file) => file.key)
+  return uniqueFiles
 }
 
 function filterBy(patterns: string[], type: string, invertMatch = false) {
@@ -51,6 +68,15 @@ export async function getPatternsFromShopifyIgnore(root: string) {
     .filter((line) => line && !line.startsWith('#'))
 }
 
+function filterRegexValues(regexList: string[]) {
+  const negatedPatterns = regexList
+    .filter((regexList) => regexList.startsWith('!'))
+    .map((regexList) => regexList.slice(1))
+  const normalPatterns = regexList.filter((regexList) => !regexList.startsWith('!'))
+
+  return [normalPatterns, negatedPatterns]
+}
+
 function matchGlob(key: string, pattern: string) {
   const matchOpts = {
     matchBase: true,

diff --git a/packages/theme/src/cli/utilities/theme-fs.test.ts b/packages/theme/src/cli/utilities/theme-fs.test.ts
@@ -293,6 +293,7 @@ describe('theme-fs', () => {
 
       // When
       const content = await readThemeFile(root, key)
+      // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
       const contentJson = JSON.parse(content?.toString() || '')
 
       // Then

diff --git a/packages/theme/src/cli/utilities/theme-fs/theme-fs-mock-factory.ts b/packages/theme/src/cli/utilities/theme-fs/theme-fs-mock-factory.ts
@@ -18,6 +18,7 @@ export function fakeThemeFileSystem(
       files.set(asset.key, asset)
     },
     read: async (fileKey: string) => {
+      // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
       return files.get(fileKey)?.value || files.get(fileKey)?.attachment
     },
     addEventListener: () => {},
-Original file line number
+Diff line change
@@ Expand Up / @@ -293,6 +293,7 @@ describe('theme-fs', () => { @@
           // When
           const content = await readThemeFile(root, key)
+          // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
           const contentJson = JSON.parse(content?.toString() || '')
           // Then
@@ Expand Down @@