Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Themes] Improve Glob Pattern subdirectory mismatch error handling #3669

Merged
merged 2 commits into from
Apr 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/spotty-badgers-attack.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/theme': patch
---

Improve Glob Pattern subdirectory mismatch error handling
33 changes: 33 additions & 0 deletions packages/theme/src/cli/utilities/asset-ignore.test.ts
Original file line number Diff line number Diff line change
@@ -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'

Expand All @@ -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 = [
Expand Down Expand Up @@ -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)
})
})
34 changes: 28 additions & 6 deletions packages/theme/src/cli/utilities/asset-ignore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decision: I chose to limit this to the templates folder because that's the only folder where subdirectories are supported.

Some questions that could influence this

  • Do you consider something like assets/*.liquid to be unsafe / something we need to substitute?

    • If we recommend assets/**/*.liquid, that could be confusing since there are no subdirectories there
  • Will we remove the automatic conversion of the glob pattern one day so that templates/*.json and templates/**/*.json mean different things in the future?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for restricting this warning to templates, as it's the only directory that may have a sub-directory :)

Regarding the removal of this warning in the future, because it would be a destructive breaking change (as in some contexts it may imply file deletion), I'd suggest making this change in the next major release

return pattern.includes('/*.') && !pattern.includes('/**/*.') && pattern.includes('templates/')
}

function regexMatch(key: string, pattern: string) {
try {
return key.match(pattern)
Expand Down