Skip to content

Commit 8cc6cc8

Browse files
vividvioletvividviolet
committed
Fix dev issues with manifest generation and files copying
Include built assets in the manifest.json Allow serving static assets from the extensions directory
1 parent 6654cf1 commit 8cc6cc8

10 files changed

Lines changed: 182 additions & 124 deletions

File tree

‎packages/app/src/cli/models/extensions/specifications/ui_extension.test.ts‎

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,7 @@ describe('ui_extension', async () => {
208208
target: 'EXTENSION::POINT::A',
209209
tools: undefined,
210210
instructions: undefined,
211+
intents: undefined,
211212
module: './src/ExtensionPointA.js',
212213
metafields: [{namespace: 'test', key: 'test'}],
213214
default_placement_reference: undefined,
@@ -275,6 +276,7 @@ describe('ui_extension', async () => {
275276
target: 'EXTENSION::POINT::A',
276277
tools: undefined,
277278
instructions: undefined,
279+
intents: undefined,
278280
module: './src/ExtensionPointA.js',
279281
metafields: [],
280282
default_placement_reference: 'PLACEMENT_REFERENCE1',
@@ -338,6 +340,7 @@ describe('ui_extension', async () => {
338340
target: 'EXTENSION::POINT::A',
339341
tools: undefined,
340342
instructions: undefined,
343+
intents: undefined,
341344
module: './src/ExtensionPointA.js',
342345
metafields: [],
343346
urls: {},
@@ -401,6 +404,7 @@ describe('ui_extension', async () => {
401404
target: 'EXTENSION::POINT::A',
402405
tools: undefined,
403406
instructions: undefined,
407+
intents: undefined,
404408
module: './src/ExtensionPointA.js',
405409
metafields: [],
406410
default_placement_reference: undefined,
@@ -467,6 +471,7 @@ describe('ui_extension', async () => {
467471
target: 'EXTENSION::POINT::A',
468472
tools: undefined,
469473
instructions: undefined,
474+
intents: undefined,
470475
module: './src/ExtensionPointA.js',
471476
metafields: [],
472477
default_placement_reference: undefined,
@@ -535,6 +540,7 @@ describe('ui_extension', async () => {
535540
target: 'EXTENSION::POINT::A',
536541
tools: undefined,
537542
instructions: undefined,
543+
intents: undefined,
538544
module: './src/ExtensionPointA.js',
539545
metafields: [],
540546
default_placement_reference: undefined,
@@ -603,6 +609,7 @@ describe('ui_extension', async () => {
603609
module: './src/ExtensionPointA.js',
604610
tools: './tools.json',
605611
instructions: undefined,
612+
intents: undefined,
606613
metafields: [],
607614
default_placement_reference: undefined,
608615
capabilities: undefined,
@@ -613,11 +620,6 @@ describe('ui_extension', async () => {
613620
filepath: 'test-ui-extension.js',
614621
module: './src/ExtensionPointA.js',
615622
},
616-
tools: {
617-
filepath: 'test-ui-extension-tools-tools.json',
618-
module: './tools.json',
619-
static: true,
620-
},
621623
},
622624
},
623625
urls: {},
@@ -671,6 +673,7 @@ describe('ui_extension', async () => {
671673
module: './src/ExtensionPointA.js',
672674
tools: undefined,
673675
instructions: './instructions.md',
676+
intents: undefined,
674677
metafields: [],
675678
default_placement_reference: undefined,
676679
capabilities: undefined,
@@ -681,11 +684,6 @@ describe('ui_extension', async () => {
681684
filepath: 'test-ui-extension.js',
682685
module: './src/ExtensionPointA.js',
683686
},
684-
instructions: {
685-
filepath: 'test-ui-extension-instructions-instructions.md',
686-
module: './instructions.md',
687-
static: true,
688-
},
689687
},
690688
},
691689
urls: {},
@@ -899,6 +897,7 @@ Please check the configuration in ${joinPath(tmpDir, 'shopify.extension.toml')}`
899897
module: './src/ExtensionPointA.js',
900898
tools: './tools.json',
901899
instructions: './instructions.md',
900+
intents: undefined,
902901
metafields: [],
903902
default_placement_reference: undefined,
904903
capabilities: undefined,
@@ -909,16 +908,6 @@ Please check the configuration in ${joinPath(tmpDir, 'shopify.extension.toml')}`
909908
filepath: 'test-ui-extension.js',
910909
module: './src/ExtensionPointA.js',
911910
},
912-
tools: {
913-
filepath: 'test-ui-extension-tools-tools.json',
914-
module: './tools.json',
915-
static: true,
916-
},
917-
instructions: {
918-
filepath: 'test-ui-extension-instructions-instructions.md',
919-
module: './instructions.md',
920-
static: true,
921-
},
922911
},
923912
},
924913
urls: {},

‎packages/app/src/cli/models/extensions/specifications/ui_extension.ts‎

Lines changed: 13 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import {ExtensionInstance} from '../extension-instance.js'
1414
import {formatContent} from '../../../utilities/file-formatter.js'
1515
import {err, ok, Result} from '@shopify/cli-kit/node/result'
1616
import {copyFile, fileExists, readFile} from '@shopify/cli-kit/node/fs'
17-
import {joinPath, basename, dirname} from '@shopify/cli-kit/node/path'
17+
import {joinPath, dirname} from '@shopify/cli-kit/node/path'
1818
import {outputContent, outputToken, outputWarn} from '@shopify/cli-kit/node/output'
1919
import {zod} from '@shopify/cli-kit/node/schema'
2020

@@ -61,24 +61,6 @@ export const UIExtensionSchema = BaseSchema.extend({
6161
},
6262
}
6363
: null),
64-
...(targeting.tools
65-
? {
66-
[AssetIdentifier.Tools]: {
67-
filepath: `${config.handle}-${AssetIdentifier.Tools}-${basename(targeting.tools)}`,
68-
module: targeting.tools,
69-
static: true,
70-
},
71-
}
72-
: null),
73-
...(targeting.instructions
74-
? {
75-
[AssetIdentifier.Instructions]: {
76-
filepath: `${config.handle}-${AssetIdentifier.Instructions}-${basename(targeting.instructions)}`,
77-
module: targeting.instructions,
78-
static: true,
79-
},
80-
}
81-
: null),
8264
},
8365
}
8466

@@ -93,6 +75,7 @@ export const UIExtensionSchema = BaseSchema.extend({
9375
build_manifest: buildManifest,
9476
tools: targeting.tools,
9577
instructions: targeting.instructions,
78+
intents: targeting.intents,
9679
}
9780
})
9881
return {...config, extension_points: extensionPoints}
@@ -115,24 +98,25 @@ const uiExtensionSpec = createExtensionSpecification({
11598
type: 'include_assets',
11699
config: {
117100
generatesAssetsManifest: true,
101+
builtAssets: {anchor: 'extension_points[]', groupBy: 'target', key: 'build_manifest'},
118102
inclusions: [
119103
{
120104
type: 'configKey',
121-
anchor: 'targeting[]',
105+
anchor: 'extension_points[]',
122106
groupBy: 'target',
123-
key: 'targeting[].tools',
107+
key: 'extension_points[].tools',
124108
},
125109
{
126110
type: 'configKey',
127-
anchor: 'targeting[]',
111+
anchor: 'extension_points[]',
128112
groupBy: 'target',
129-
key: 'targeting[].instructions',
113+
key: 'extension_points[].instructions',
130114
},
131115
{
132116
type: 'configKey',
133-
anchor: 'targeting[]',
117+
anchor: 'extension_points[]',
134118
groupBy: 'target',
135-
key: 'targeting[].intents[].schema',
119+
key: 'extension_points[].intents[].schema',
136120
},
137121
],
138122
},
@@ -415,28 +399,23 @@ async function validateUIExtensionPointConfig(
415399
}
416400

417401
for await (const extensionPoint of extensionPoints) {
418-
const {module, target, build_manifest: buildManifest} = extensionPoint
402+
const {module, target} = extensionPoint
419403

420404
const missingModuleError = await checkForMissingPath(directory, module, target, 'module')
421405
if (missingModuleError) {
422406
errors.push(missingModuleError)
423407
}
424408

425-
const missingToolsError = await checkForMissingPath(
426-
directory,
427-
buildManifest?.assets[AssetIdentifier.Tools]?.module,
428-
target,
429-
AssetIdentifier.Tools,
430-
)
409+
const missingToolsError = await checkForMissingPath(directory, extensionPoint.tools, target, 'tools')
431410
if (missingToolsError) {
432411
errors.push(missingToolsError)
433412
}
434413

435414
const missingInstructionsError = await checkForMissingPath(
436415
directory,
437-
buildManifest?.assets[AssetIdentifier.Instructions]?.module,
416+
extensionPoint.instructions,
438417
target,
439-
AssetIdentifier.Instructions,
418+
'instructions',
440419
)
441420
if (missingInstructionsError) {
442421
errors.push(missingInstructionsError)

‎packages/app/src/cli/services/build/steps/include-assets-step.test.ts‎

Lines changed: 26 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -290,8 +290,7 @@ describe('executeIncludeAssetsStep', () => {
290290
)
291291
})
292292

293-
test('renames output file to avoid collision when candidate path already exists', async () => {
294-
// Given — tools.json already exists in the output dir; findUniqueDestPath must try tools-1.json
293+
test('overwrites existing file on rebuild', async () => {
295294
const contextWithConfig = {
296295
...mockContext,
297296
extension: {
@@ -302,7 +301,7 @@ describe('executeIncludeAssetsStep', () => {
302301

303302
vi.mocked(fs.fileExists).mockImplementation(async (path) => {
304303
const pathStr = String(path)
305-
// Source file exists; first candidate output path is taken; suffixed path is free
304+
// Source file exists; output file also exists from previous build
306305
return pathStr === '/test/extension/tools.json' || pathStr === '/test/output/tools.json'
307306
})
308307
vi.mocked(fs.isDirectory).mockResolvedValue(false)
@@ -318,45 +317,47 @@ describe('executeIncludeAssetsStep', () => {
318317
},
319318
}
320319

321-
// When
322320
const result = await executeIncludeAssetsStep(step, contextWithConfig)
323321

324-
// Then — copied to tools-1.json, not tools.json
325-
expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/tools.json', '/test/output/tools-1.json')
322+
// Overwrites the existing file rather than creating tools-1.json
323+
expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/tools.json', '/test/output/tools.json')
326324
expect(result.filesCopied).toBe(1)
327325
})
328326

329-
test('throws after exhausting 1000 rename attempts', async () => {
330-
// Given — every candidate path (tools.json, tools-1.json … tools-1000.json) is taken
327+
test('renames file to avoid collision when two different sources share the same basename', async () => {
331328
const contextWithConfig = {
332329
...mockContext,
333330
extension: {
334331
...mockExtension,
335-
configuration: {tools: './tools.json'},
332+
configuration: {tools_a: './a/schema.json', tools_b: './b/schema.json'},
336333
} as unknown as ExtensionInstance,
337334
}
338335

339336
vi.mocked(fs.fileExists).mockImplementation(async (path) => {
340337
const pathStr = String(path)
341-
// Source file exists; all 1001 output candidates are occupied
342-
return pathStr.startsWith('/test/extension/') || pathStr.startsWith('/test/output/tools')
338+
return pathStr === '/test/extension/a/schema.json' || pathStr === '/test/extension/b/schema.json'
343339
})
344340
vi.mocked(fs.isDirectory).mockResolvedValue(false)
341+
vi.mocked(fs.copyFile).mockResolvedValue()
345342
vi.mocked(fs.mkdir).mockResolvedValue()
346343

347344
const step: LifecycleStep = {
348345
id: 'copy-tools',
349346
name: 'Copy Tools',
350347
type: 'include_assets',
351348
config: {
352-
inclusions: [{type: 'configKey', key: 'tools'}],
349+
inclusions: [
350+
{type: 'configKey', key: 'tools_a'},
351+
{type: 'configKey', key: 'tools_b'},
352+
],
353353
},
354354
}
355355

356-
// When / Then
357-
await expect(executeIncludeAssetsStep(step, contextWithConfig)).rejects.toThrow(
358-
"Unable to find unique destination path for 'tools.json' in '/test/output' after 1000 attempts",
359-
)
356+
const result = await executeIncludeAssetsStep(step, contextWithConfig)
357+
358+
expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/a/schema.json', '/test/output/schema.json')
359+
expect(fs.copyFile).toHaveBeenCalledWith('/test/extension/b/schema.json', '/test/output/schema-1.json')
360+
expect(result.filesCopied).toBe(2)
360361
})
361362

362363
test('resolves array config value and copies each path', async () => {
@@ -664,9 +665,7 @@ describe('executeIncludeAssetsStep', () => {
664665
beforeEach(() => {
665666
vi.mocked(fs.writeFile).mockResolvedValue()
666667
vi.mocked(fs.mkdir).mockResolvedValue()
667-
// Source files exist; destination paths don't yet (so findUniqueDestPath
668-
// resolves on the first candidate without looping). Individual tests can
669-
// override for specific scenarios.
668+
// Source files exist. Individual tests can override for specific scenarios.
670669
vi.mocked(fs.fileExists).mockImplementation(
671670
async (path) => typeof path === 'string' && path.startsWith('/test/extension'),
672671
)
@@ -1044,8 +1043,8 @@ describe('executeIncludeAssetsStep', () => {
10441043
)
10451044
})
10461045

1047-
test('throws when manifest.json already exists in the output directory', async () => {
1048-
// Given — a prior inclusion already copied a manifest.json to the output dir
1046+
test('overwrites manifest.json when it already exists in the output directory', async () => {
1047+
// Given — a prior build already generated a manifest.json in the output dir
10491048
const contextWithConfig = {
10501049
...mockContext,
10511050
extension: {
@@ -1056,7 +1055,7 @@ describe('executeIncludeAssetsStep', () => {
10561055
} as unknown as ExtensionInstance,
10571056
}
10581057

1059-
// Source files exist; output manifest.json already exists (simulating conflict);
1058+
// Source files exist; output manifest.json already exists from previous build;
10601059
// candidate output paths for tools.json are free so copyConfigKeyEntry succeeds.
10611060
vi.mocked(fs.fileExists).mockImplementation(async (path) => {
10621061
const pathStr = String(path)
@@ -1081,10 +1080,11 @@ describe('executeIncludeAssetsStep', () => {
10811080
},
10821081
}
10831082

1084-
// When / Then — throws rather than silently overwriting
1085-
await expect(executeIncludeAssetsStep(step, contextWithConfig)).rejects.toThrow(
1086-
`Can't write manifest.json: a file already exists at '/test/output/manifest.json'`,
1087-
)
1083+
// When / Then — overwrites the existing manifest.json
1084+
await executeIncludeAssetsStep(step, contextWithConfig)
1085+
expect(fs.writeFile).toHaveBeenCalledOnce()
1086+
const writeFileCall = vi.mocked(fs.writeFile).mock.calls[0]!
1087+
expect(writeFileCall[0]).toBe('/test/output/manifest.json')
10881088
})
10891089

10901090
test('writes an empty manifest when anchor resolves to a non-array value', async () => {

0 commit comments

Comments
 (0)