Skip to content

refactor: remove redundant options param from copyConfigKeyEntry #7146

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.

Sign up for GitHub

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

Merged
craigmichaelmartin merged 1 commit into from
Apr 2, 2026
Merged

refactor: remove redundant options param from copyConfigKeyEntry #7146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 7 additions & 10 deletions packages/app/src/cli/services/build/steps/include-assets-step.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,16 +140,13 @@ export async function executeIncludeAssetsStep(
const rawDest = entry.destination ? sanitizeRelativePath(entry.destination, warn) : undefined
const sanitizedDest = rawDest === '' ? undefined : rawDest
// eslint-disable-next-line no-await-in-loop
const result = await copyConfigKeyEntry(
{
key: entry.key,
baseDir: extension.directory,
outputDir,
context,
destination: sanitizedDest,
},
options,
)
const result = await copyConfigKeyEntry({
key: entry.key,
baseDir: extension.directory,
outputDir,
context,
destination: sanitizedDest,
})
result.pathMap.forEach((val, key) => aggregatedPathMap.set(key, val))
configKeyCount += result.filesCopied
}
Expand Down
98 changes: 46 additions & 52 deletions packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ import {inTemporaryDirectory, writeFile, fileExists, mkdir, readFile} from '@sho
import {joinPath} from '@shopify/cli-kit/node/path'
import {describe, expect, test, vi, beforeEach} from 'vitest'

const makeContext = (configuration: Record<string, unknown>) => ({
const makeContext = (configuration: Record<string, unknown>, stdout: any = {write: vi.fn()}) => ({
extension: {configuration} as any,
options: {} as any,
options: {stdout} as any,
stepResults: new Map(),
})

Expand All @@ -26,11 +26,8 @@ describe('copyConfigKeyEntry', () => {
const outDir = joinPath(tmpDir, 'out')
await mkdir(outDir)

const context = makeContext({static_root: 'public'})
const result = await copyConfigKeyEntry(
{key: 'static_root', baseDir: tmpDir, outputDir: outDir, context},
{stdout: mockStdout},
)
const context = makeContext({static_root: 'public'}, mockStdout)
const result = await copyConfigKeyEntry({key: 'static_root', baseDir: tmpDir, outputDir: outDir, context})

expect(result.filesCopied).toBe(2)
await expect(fileExists(joinPath(outDir, 'index.html'))).resolves.toBe(true)
Expand All @@ -49,11 +46,8 @@ describe('copyConfigKeyEntry', () => {
const outDir = joinPath(tmpDir, 'out')
await mkdir(outDir)

const context = makeContext({schema_path: 'src/schema.json'})
const result = await copyConfigKeyEntry(
{key: 'schema_path', baseDir: tmpDir, outputDir: outDir, context},
{stdout: mockStdout},
)
const context = makeContext({schema_path: 'src/schema.json'}, mockStdout)
const result = await copyConfigKeyEntry({key: 'schema_path', baseDir: tmpDir, outputDir: outDir, context})

expect(result.filesCopied).toBe(1)
await expect(fileExists(joinPath(outDir, 'schema.json'))).resolves.toBe(true)
Expand All @@ -74,11 +68,8 @@ describe('copyConfigKeyEntry', () => {
// Pre-create the first candidate to force a rename
await writeFile(joinPath(outDir, 'tools-a.json'), 'existing')

const context = makeContext({files: ['tools-a.json', 'tools-b.json']})
const result = await copyConfigKeyEntry(
{key: 'files', baseDir: tmpDir, outputDir: outDir, context},
{stdout: mockStdout},
)
const context = makeContext({files: ['tools-a.json', 'tools-b.json']}, mockStdout)
const result = await copyConfigKeyEntry({key: 'files', baseDir: tmpDir, outputDir: outDir, context})

expect(result.filesCopied).toBe(2)
// tools-a.json was taken, so the copy lands as tools-a-1.json
Expand All @@ -92,11 +83,8 @@ describe('copyConfigKeyEntry', () => {
const outDir = joinPath(tmpDir, 'out')
await mkdir(outDir)

const context = makeContext({})
const result = await copyConfigKeyEntry(
{key: 'static_root', baseDir: tmpDir, outputDir: outDir, context},
{stdout: mockStdout},
)
const context = makeContext({}, mockStdout)
const result = await copyConfigKeyEntry({key: 'static_root', baseDir: tmpDir, outputDir: outDir, context})

expect(result.filesCopied).toBe(0)
expect(result.pathMap.size).toBe(0)
Expand All @@ -109,11 +97,8 @@ describe('copyConfigKeyEntry', () => {
await mkdir(outDir)

// 'nonexistent' directory is NOT created, so fileExists returns false naturally
const context = makeContext({assets_dir: 'nonexistent'})
const result = await copyConfigKeyEntry(
{key: 'assets_dir', baseDir: tmpDir, outputDir: outDir, context},
{stdout: mockStdout},
)
const context = makeContext({assets_dir: 'nonexistent'}, mockStdout)
const result = await copyConfigKeyEntry({key: 'assets_dir', baseDir: tmpDir, outputDir: outDir, context})

expect(result.filesCopied).toBe(0)
expect(result.pathMap.size).toBe(0)
Expand All @@ -137,11 +122,8 @@ describe('copyConfigKeyEntry', () => {
const outDir = joinPath(tmpDir, 'out')
await mkdir(outDir)

const context = makeContext({roots: ['public', 'assets']})
const result = await copyConfigKeyEntry(
{key: 'roots', baseDir: tmpDir, outputDir: outDir, context},
{stdout: mockStdout},
)
const context = makeContext({roots: ['public', 'assets']}, mockStdout)
const result = await copyConfigKeyEntry({key: 'roots', baseDir: tmpDir, outputDir: outDir, context})

// Promise.all runs copies sequentially; glob on the shared outDir may see files
// from the other copy, so the total count is at least 3 (one per real file).
Expand All @@ -161,11 +143,14 @@ describe('copyConfigKeyEntry', () => {
const outDir = joinPath(tmpDir, 'out')
await mkdir(outDir)

const context = makeContext({icons_dir: 'icons'})
await copyConfigKeyEntry(
{key: 'icons_dir', baseDir: tmpDir, outputDir: outDir, context, destination: 'static/icons'},
{stdout: mockStdout},
)
const context = makeContext({icons_dir: 'icons'}, mockStdout)
await copyConfigKeyEntry({
key: 'icons_dir',
baseDir: tmpDir,
outputDir: outDir,
context,
destination: 'static/icons',
})

await expect(fileExists(joinPath(outDir, 'static', 'icons', 'icon.svg'))).resolves.toBe(true)
})
Expand All @@ -186,10 +171,12 @@ describe('copyConfigKeyEntry', () => {
{targeting: [{schema: 'schema-c.json'}]},
],
})
const result = await copyConfigKeyEntry(
{key: 'extensions[].targeting[].schema', baseDir: tmpDir, outputDir: outDir, context},
{stdout: mockStdout},
)
const result = await copyConfigKeyEntry({
key: 'extensions[].targeting[].schema',
baseDir: tmpDir,
outputDir: outDir,
context,
})

expect(result.filesCopied).toBe(3)
await expect(fileExists(joinPath(outDir, 'schema-a.json'))).resolves.toBe(true)
Expand All @@ -203,15 +190,20 @@ describe('copyConfigKeyEntry', () => {
const outDir = joinPath(tmpDir, 'out')
await mkdir(outDir)

const context = makeContext({
extensions: {targeting: {schema: 'schema.json'}},
})

const result = await copyConfigKeyEntry(
{key: 'extensions[].targeting[].schema', baseDir: tmpDir, outputDir: outDir, context},
{stdout: mockStdout},
const context = makeContext(
{
extensions: {targeting: {schema: 'schema.json'}},
},
mockStdout,
)

const result = await copyConfigKeyEntry({
key: 'extensions[].targeting[].schema',
baseDir: tmpDir,
outputDir: outDir,
context,
})

expect(result.filesCopied).toBe(0)
expect(result.pathMap.size).toBe(0)
})
Expand All @@ -228,10 +220,12 @@ describe('copyConfigKeyEntry', () => {
const context = makeContext({
extensions: [{targeting: [{tools: 'tools.json'}, {tools: 'tools.json'}]}],
})
const result = await copyConfigKeyEntry(
{key: 'extensions[].targeting[].tools', baseDir: tmpDir, outputDir: outDir, context},
{stdout: mockStdout},
)
const result = await copyConfigKeyEntry({
key: 'extensions[].targeting[].tools',
baseDir: tmpDir,
outputDir: outDir,
context,
})

expect(result.filesCopied).toBe(1)
await expect(fileExists(joinPath(outDir, 'tools.json'))).resolves.toBe(true)
Expand Down
26 changes: 12 additions & 14 deletions packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,15 @@ import type {BuildContext} from '../../client-steps.js'
* value to its output-relative location. File sources map to a single string.
* Directory sources map to a `string[]` of every output-relative file path.
*/
export async function copyConfigKeyEntry(
config: {
key: string
baseDir: string
outputDir: string
context: BuildContext
destination?: string
},
options: {stdout: NodeJS.WritableStream},
): Promise<{filesCopied: number; pathMap: Map<string, string | string[]>}> {
export async function copyConfigKeyEntry(config: {
key: string
baseDir: string
outputDir: string
context: BuildContext
destination?: string
}): Promise<{filesCopied: number; pathMap: Map<string, string | string[]>}> {
const {key, baseDir, outputDir, context, destination} = config
const {stdout} = context.options
const value = getNestedValue(context.extension.configuration, key)
let paths: string[]
if (typeof value === 'string') {
Expand All @@ -42,7 +40,7 @@ export async function copyConfigKeyEntry(
}

if (paths.length === 0) {
outputDebug(`No value for configKey '${key}', skipping\n`, context.options.stdout)
outputDebug(`No value for configKey '${key}', skipping\n`, stdout)
return {filesCopied: 0, pathMap: new Map()}
}

Expand All @@ -62,7 +60,7 @@ export async function copyConfigKeyEntry(
const fullPath = joinPath(baseDir, sourcePath)
const exists = await fileExists(fullPath)
if (!exists) {
options.stdout.write(`Warning: path '${sourcePath}' does not exist, skipping\n`)
stdout.write(`Warning: path '${sourcePath}' does not exist, skipping\n`)
continue
}

Expand All @@ -73,7 +71,7 @@ export async function copyConfigKeyEntry(
if (sourceIsDir) {
await copyDirectoryContents(fullPath, destDir)
const copied = await glob(['**/*'], {cwd: destDir, absolute: false})
options.stdout.write(`Included '${sourcePath}'\n`)
stdout.write(`Included '${sourcePath}'\n`)
const relFiles = copied.map((file) => relativePath(outputDir, joinPath(destDir, file)))
pathMap.set(sourcePath, relFiles)
filesCopied += copied.length
Expand All @@ -82,7 +80,7 @@ export async function copyConfigKeyEntry(
const uniqueDestPath = await findUniqueDestPath(destDir, basename(fullPath))
await copyFile(fullPath, uniqueDestPath)
const outputRelative = relativePath(outputDir, uniqueDestPath)
options.stdout.write(`Included '${sourcePath}'\n`)
stdout.write(`Included '${sourcePath}'\n`)
pathMap.set(sourcePath, outputRelative)
filesCopied += 1
}
Expand Down
Loading