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

Add .shopify/metafields.json to .gitignore by default #5184

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

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

When the `.shopify/metafields.json` file gets created, the CLI now proposes to add it to `.gitignore` by default
39 changes: 39 additions & 0 deletions packages/cli-kit/src/public/node/fs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,17 @@ import {
generateRandomNameForSubdirectory,
readFileSync,
glob,
detectEOL,
} from './fs.js'
import {joinPath} from './path.js'
import {takeRandomFromArray} from '../common/array.js'
import {describe, expect, test, vi} from 'vitest'
import FastGlob from 'fast-glob'
import * as os from 'os'

vi.mock('../common/array.js')
vi.mock('fast-glob')
vi.mock('os')

describe('inTemporaryDirectory', () => {
test('ties the lifecycle of the temporary directory to the lifecycle of the callback', async () => {
Expand Down Expand Up @@ -283,3 +286,39 @@ describe('glob', () => {
expect(FastGlob).toBeCalledWith('pattern', {dot: false})
})
})

describe('detectEOL', () => {
test('detects the EOL of a file', async () => {
// Given
const fileContent = 'test\ncontent'

// When
const eol = detectEOL(fileContent)

// Then
expect(eol).toEqual('\n')
})

test('detects the EOL of a file with CRLF', async () => {
// Given
const fileContent = 'test\r\ncontent'

// When
const eol = detectEOL(fileContent)

// Then
expect(eol).toEqual('\r\n')
})

test('returns the default EOL if no EOL is found', async () => {
// Given
const fileContent = 'testcontent'
vi.mocked(os).EOL = '\n'

// When
const eol = detectEOL(fileContent)

// Then
expect(eol).toEqual('\n')
})
})
37 changes: 37 additions & 0 deletions packages/cli-kit/src/public/node/fs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,43 @@ export async function glob(pattern: Pattern | Pattern[], options?: GlobOptions):
export function pathToFileURL(path: string): URL {
return pathToFile(path)
}

/**
* The operating system-specific end-of-line marker:
* - `\n` on POSIX
* - `\r\n` on Windows
*/
export type EOL = '\r\n' | '\n'

/**
* Detects the end-of-line marker used in a string.
*
* @param content - file contents to analyze
*
* @returns The detected end-of-line marker
*/
export function detectEOL(content: string): EOL {
const match = content.match(/\r\n|\n/g)

if (!match) {
return defaultEOL()
}

const crlf = match.filter((eol) => eol === '\r\n').length
const lf = match.filter((eol) => eol === '\n').length

return crlf > lf ? '\r\n' : '\n'
}

/**
* Returns the operating system's end-of-line marker.
*
* @returns The OS-specific end-of-line marker
*/
export function defaultEOL(): EOL {
return os.EOL as EOL
}

/**
* Find a file by walking parent directories.
*
Expand Down
25 changes: 24 additions & 1 deletion packages/theme/src/cli/services/metafields-pull.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {AdminSession, ensureAuthenticatedThemes} from '@shopify/cli-kit/node/ses
import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output'
import {metafieldDefinitionsByOwnerType} from '@shopify/cli-kit/node/themes/api'
import {describe, test, vi, beforeEach, expect, afterEach} from 'vitest'
import {fileExists, inTemporaryDirectory, readFile} from '@shopify/cli-kit/node/fs'
import {fileExists, inTemporaryDirectory, readFile, writeFileSync} from '@shopify/cli-kit/node/fs'

vi.mock('../utilities/theme-store.js')
vi.mock('../utilities/theme-ui.js')
Expand Down Expand Up @@ -83,6 +83,29 @@ describe('metafields-pull', () => {
expect(capturedOutput.error()).toBeFalsy()
})

test('should add metafields to .gitignore', async () => {
// Given
vi.mocked(metafieldDefinitionsByOwnerType).mockImplementation(() => {
return Promise.resolve([])
})

await inTemporaryDirectory(async (tmpDir) => {
// Given
const gitIgnorePath = `${tmpDir}/.gitignore`
writeFileSync(gitIgnorePath, '.DS_Store\n.shopify/secrets.json')

// When
await metafieldsPull({path: tmpDir})

// Then
await expect(fileExists(gitIgnorePath)).resolves.toBe(true)
await expect(readFile(gitIgnorePath)).resolves.toBe(`.DS_Store\n.shopify/secrets.json\n.shopify`)
})

expect(capturedOutput.info()).toContain('Metafield definitions have been successfully downloaded.')
expect(capturedOutput.error()).toBeFalsy()
})

test('should output to debug if some metafield definitions are not found', async () => {
// Given
vi.mocked(metafieldDefinitionsByOwnerType).mockImplementation((type: any, _session: AdminSession) => {
Expand Down
29 changes: 27 additions & 2 deletions packages/theme/src/cli/services/metafields-pull.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {AdminSession, ensureAuthenticatedThemes} from '@shopify/cli-kit/node/ses
import {cwd, joinPath} from '@shopify/cli-kit/node/path'
import {metafieldDefinitionsByOwnerType} from '@shopify/cli-kit/node/themes/api'
import {renderError, renderSuccess} from '@shopify/cli-kit/node/ui'
import {mkdirSync, writeFileSync} from '@shopify/cli-kit/node/fs'
import {detectEOL, fileExistsSync, mkdirSync, readFileSync, writeFileSync} from '@shopify/cli-kit/node/fs'
import {outputDebug} from '@shopify/cli-kit/node/output'

interface MetafieldsPullOptions {
Expand Down Expand Up @@ -161,5 +161,30 @@ function writeMetafieldDefinitionsToFile(path: string, content: unknown) {
mkdirSync(shopifyDirectory)

const filePath = joinPath(shopifyDirectory, 'metafields.json')
writeFileSync(filePath, JSON.stringify(content, null, 2))
const fileContent = JSON.stringify(content, null, 2)

if (!fileExistsSync(filePath)) {
addToGitIgnore(path, '.shopify')
}
karreiro marked this conversation as resolved.
Show resolved Hide resolved

writeFileSync(filePath, fileContent)
}

function addToGitIgnore(root: string, entry: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

const gitIgnorePath = joinPath(root, '.gitignore')

if (!fileExistsSync(gitIgnorePath)) {
// When the .gitignore file does not exist, the CLI should not be opinionated about creating it
return
}

const gitIgnoreContent = readFileSync(gitIgnorePath).toString()
const eol = detectEOL(gitIgnoreContent)

if (gitIgnoreContent.split(eol).includes(entry)) {
// The file already existing in the .gitignore
return
}

writeFileSync(gitIgnorePath, `${gitIgnoreContent}${eol}${entry}`)
}
Loading