Skip to content

Commit 1e8963e

Browse files
NathanPJFclaude
andcommitted
Upload settings_schema.json before validator-consumer assets
The CLI uploaded `config/settings_schema.json` and `config/settings_data.json` together as the LAST batch of theme files. Server-side validators on blocks, sections, section-group JSON, and template JSON resolve dynamic-source defaults of the form `{{ settings.<theme_setting>.* }}` against the theme's currently-stored schema. On the first push to a fresh dev theme that schema is empty, so any asset whose schema references a not-yet-declared theme setting fails validation — the user sees errors like "Invalid schema: setting with id=... default must be a color or dynamic source access path" or "Dynamic source default value added to '...' is invalid". A second push then succeeds because the user's real schema is now on the server. Split the `configFiles` partition bucket into `configSchemaFile` and `configDataFile` and reorder the dependent upload chain so schema lands first and data lands last. Schema has no upstream dependencies; data legitimately needs to be last because its current and presets validate against the freshly-uploaded schema. The matching delete order is inverted accordingly. Closes shop/issues-merchant-workflows#1929 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 95c92ff commit 1e8963e

5 files changed

Lines changed: 124 additions & 42 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@shopify/theme': patch
3+
---
4+
5+
Upload `config/settings_schema.json` before block, section, section-group, and template assets so dynamic-source defaults referencing newly declared theme-level settings validate correctly on the first push to a fresh dev theme.

packages/theme/src/cli/utilities/theme-fs.test.ts

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -473,7 +473,8 @@ describe('theme-fs', () => {
473473
otherLiquidFiles,
474474
templateJsonFiles,
475475
otherJsonFiles,
476-
configFiles,
476+
configSchemaFile,
477+
configDataFile,
477478
staticAssetFiles,
478479
contextualizedJsonFiles,
479480
blockLiquidFiles,
@@ -489,10 +490,8 @@ describe('theme-fs', () => {
489490
])
490491
expect(otherJsonFiles).toEqual([{key: 'locales/en.default.json', checksum: '6'}])
491492
expect(templateJsonFiles).toEqual([{key: 'templates/404.json', checksum: '7'}])
492-
expect(configFiles).toEqual([
493-
{key: 'config/settings_schema.json', checksum: '8'},
494-
{key: 'config/settings_data.json', checksum: '9'},
495-
])
493+
expect(configSchemaFile).toEqual([{key: 'config/settings_schema.json', checksum: '8'}])
494+
expect(configDataFile).toEqual([{key: 'config/settings_data.json', checksum: '9'}])
496495
expect(staticAssetFiles).toEqual([
497496
{key: 'assets/base.css', checksum: '1'},
498497
{key: 'assets/sparkle.gif', checksum: '3'},
@@ -511,15 +510,23 @@ describe('theme-fs', () => {
511510
const files: Checksum[] = []
512511

513512
// When
514-
const {sectionLiquidFiles, otherLiquidFiles, templateJsonFiles, otherJsonFiles, configFiles, staticAssetFiles} =
515-
partitionThemeFiles(files)
513+
const {
514+
sectionLiquidFiles,
515+
otherLiquidFiles,
516+
templateJsonFiles,
517+
otherJsonFiles,
518+
configSchemaFile,
519+
configDataFile,
520+
staticAssetFiles,
521+
} = partitionThemeFiles(files)
516522

517523
// Then
518524
expect(sectionLiquidFiles).toEqual([])
519525
expect(otherLiquidFiles).toEqual([])
520526
expect(templateJsonFiles).toEqual([])
521527
expect(otherJsonFiles).toEqual([])
522-
expect(configFiles).toEqual([])
528+
expect(configSchemaFile).toEqual([])
529+
expect(configDataFile).toEqual([])
523530
expect(staticAssetFiles).toEqual([])
524531
})
525532
})

packages/theme/src/cli/utilities/theme-fs.ts

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,8 @@ const THEME_PARTITION_REGEX = {
4444
layoutLiquidRegex: /^layout\/.+\.liquid$/,
4545
sectionLiquidRegex: /^sections\/.+\.liquid$/,
4646
blockLiquidRegex: /^blocks\/.+\.liquid$/,
47-
configRegex: /^config\/(settings_schema|settings_data)\.json$/,
47+
configSchemaRegex: /^config\/settings_schema\.json$/,
48+
configDataRegex: /^config\/settings_data\.json$/,
4849
sectionJsonRegex: /^sections\/.+\.json$/,
4950
templateJsonRegex: /^templates\/.+\.json$/,
5051
jsonRegex: /^(?!config\/).*\.json$/,
@@ -465,7 +466,8 @@ export function partitionThemeFiles<T extends {key: string}>(files: T[]) {
465466
const templateJsonFiles: T[] = []
466467
const otherJsonFiles: T[] = []
467468
const contextualizedJsonFiles: T[] = []
468-
const configFiles: T[] = []
469+
const configSchemaFile: T[] = []
470+
const configDataFile: T[] = []
469471
const staticAssetFiles: T[] = []
470472
const blockLiquidFiles: T[] = []
471473
const layoutFiles: T[] = []
@@ -482,8 +484,10 @@ export function partitionThemeFiles<T extends {key: string}>(files: T[]) {
482484
} else {
483485
otherLiquidFiles.push(file)
484486
}
485-
} else if (THEME_PARTITION_REGEX.configRegex.test(fileKey)) {
486-
configFiles.push(file)
487+
} else if (THEME_PARTITION_REGEX.configSchemaRegex.test(fileKey)) {
488+
configSchemaFile.push(file)
489+
} else if (THEME_PARTITION_REGEX.configDataRegex.test(fileKey)) {
490+
configDataFile.push(file)
487491
} else if (THEME_PARTITION_REGEX.jsonRegex.test(fileKey)) {
488492
if (THEME_PARTITION_REGEX.contextualizedJsonRegex.test(fileKey)) {
489493
contextualizedJsonFiles.push(file)
@@ -506,7 +510,8 @@ export function partitionThemeFiles<T extends {key: string}>(files: T[]) {
506510
templateJsonFiles,
507511
contextualizedJsonFiles,
508512
otherJsonFiles,
509-
configFiles,
513+
configSchemaFile,
514+
configDataFile,
510515
staticAssetFiles,
511516
blockLiquidFiles,
512517
layoutFiles,

packages/theme/src/cli/utilities/theme-uploader.test.ts

Lines changed: 63 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
import {renderTasksToStdErr} from './theme-ui.js'
22
import {fakeThemeFileSystem} from './theme-fs/theme-fs-mock-factory.js'
33
import {
4+
ChecksumWithSize,
45
MAX_BATCH_BYTESIZE,
56
MAX_BATCH_FILE_COUNT,
67
MAX_UPLOAD_RETRY_COUNT,
78
MINIMUM_THEME_ASSETS,
9+
orderFilesToBeUploaded,
810
uploadTheme,
911
updateUploadErrors,
1012
} from './theme-uploader.js'
@@ -319,17 +321,19 @@ describe('theme-uploader', () => {
319321
await renderThemeSyncProgress()
320322

321323
// Then
322-
expect(bulkUploadThemeAssets).toHaveBeenCalledTimes(9)
324+
expect(bulkUploadThemeAssets).toHaveBeenCalledTimes(10)
323325
// Minimum theme files start first
324326
expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(1, remoteTheme.id, MINIMUM_THEME_ASSETS, adminSession)
325-
// Dependent assets start second
327+
// settings_schema.json is uploaded first among dependent files so block / section /
328+
// section-group / template validators can resolve dynamic-source defaults that
329+
// reference theme-level settings declared in the schema.
326330
expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
327331
2,
328332
remoteTheme.id,
329-
[{key: 'layout/theme.liquid'}],
333+
[{key: 'config/settings_schema.json'}],
330334
adminSession,
331335
)
332-
// Independent assets start right after dependent assets start
336+
// Independent assets fan out concurrently with the dependent chain.
333337
expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
334338
3,
335339
remoteTheme.id,
@@ -346,16 +350,22 @@ describe('theme-uploader', () => {
346350
],
347351
adminSession,
348352
)
349-
// Dependent assets continue after the first batch of dependent assets ends
353+
// Layout files come after the schema is in place.
350354
expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
351355
4,
352356
remoteTheme.id,
353-
[{key: 'blocks/block.liquid'}],
357+
[{key: 'layout/theme.liquid'}],
354358
adminSession,
355359
)
356360
expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
357361
5,
358362
remoteTheme.id,
363+
[{key: 'blocks/block.liquid'}],
364+
adminSession,
365+
)
366+
expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
367+
6,
368+
remoteTheme.id,
359369
[
360370
{
361371
key: 'sections/header.liquid',
@@ -365,7 +375,7 @@ describe('theme-uploader', () => {
365375
)
366376

367377
expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
368-
6,
378+
7,
369379
remoteTheme.id,
370380
[
371381
{
@@ -376,7 +386,7 @@ describe('theme-uploader', () => {
376386
)
377387

378388
expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
379-
7,
389+
8,
380390
remoteTheme.id,
381391
[
382392
{
@@ -387,7 +397,7 @@ describe('theme-uploader', () => {
387397
)
388398

389399
expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
390-
8,
400+
9,
391401
remoteTheme.id,
392402
[
393403
{
@@ -397,21 +407,56 @@ describe('theme-uploader', () => {
397407
adminSession,
398408
)
399409

410+
// settings_data.json must be last
400411
expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
401-
9,
412+
10,
402413
remoteTheme.id,
403-
[
404-
{
405-
key: 'config/settings_data.json',
406-
},
407-
{
408-
key: 'config/settings_schema.json',
409-
},
410-
],
414+
[{key: 'config/settings_data.json'}],
411415
adminSession,
412416
)
413417
})
414418

419+
test('orders settings_schema.json first and settings_data.json last in the dependent chain, surrounding every validator-consumer asset', () => {
420+
// Regression for the fresh-theme bootstrap bug: blocks, sections, section-group
421+
// JSONs and template JSONs run server-side validators that resolve dynamic-source
422+
// defaults of the form `{{ settings.<theme_setting>.* }}` against the theme's
423+
// currently-stored settings_schema.json. If the user's new schema lands AFTER
424+
// those assets, validators see an empty schema and the first push errors out.
425+
// settings_data.json must then go LAST because its values are validated against
426+
// the freshly-uploaded schema and may reference earlier-uploaded sections/templates.
427+
const file = (key: string): ChecksumWithSize => ({key, checksum: '_', size: 0})
428+
const files: ChecksumWithSize[] = [
429+
file('config/settings_schema.json'),
430+
file('config/settings_data.json'),
431+
file('layout/theme.liquid'),
432+
file('blocks/quantity.liquid'),
433+
file('sections/header.liquid'),
434+
file('sections/header-group.json'),
435+
file('templates/product.json'),
436+
file('templates/product.context.uk.json'),
437+
]
438+
439+
const {dependentFiles} = orderFilesToBeUploaded(files)
440+
const flat = dependentFiles.flat().map((f) => f.key)
441+
442+
expect(flat.at(0)).toBe('config/settings_schema.json')
443+
expect(flat.at(-1)).toBe('config/settings_data.json')
444+
445+
const validatorConsumers = [
446+
'blocks/quantity.liquid',
447+
'sections/header.liquid',
448+
'sections/header-group.json',
449+
'templates/product.json',
450+
'templates/product.context.uk.json',
451+
]
452+
for (const key of validatorConsumers) {
453+
const index = flat.indexOf(key)
454+
expect(index, `${key} missing from dependent chain`).not.toBe(-1)
455+
expect(index, `${key} should be after settings_schema.json`).toBeGreaterThan(0)
456+
expect(index, `${key} should be before settings_data.json`).toBeLessThan(flat.length - 1)
457+
}
458+
})
459+
415460
test('should create batches for files when bulk upload file count limit is reached', async () => {
416461
// Given
417462
const remoteChecksums: Checksum[] = []

packages/theme/src/cli/utilities/theme-uploader.ts

Lines changed: 31 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ interface UploadOptions {
2020
multiEnvironment?: boolean
2121
}
2222

23-
type ChecksumWithSize = Checksum & {size: number}
23+
export type ChecksumWithSize = Checksum & {size: number}
2424
type FileBatch = ChecksumWithSize[]
2525

2626
/**
@@ -213,7 +213,9 @@ function orderFilesToBeDeleted(files: Checksum[]): Checksum[] {
213213
...fileSets.blockLiquidFiles,
214214
...fileSets.layoutFiles,
215215
...fileSets.otherLiquidFiles,
216-
...fileSets.configFiles,
216+
// Inverse of the upload order: data first (consumes schema), then schema.
217+
...fileSets.configDataFile,
218+
...fileSets.configSchemaFile,
217219
...fileSets.staticAssetFiles,
218220
]
219221
}
@@ -311,21 +313,34 @@ function selectUploadableFiles(themeFileSystem: ThemeFileSystem, remoteChecksums
311313
* We use this 2d array to batch files of the same type together
312314
* while maintaining the order between file types. The files with
313315
* dependencies we have are:
314-
* 1. Layout files don't necessarily need to be the first, but they must uploaded before templates.
315-
* 2. Liquid blocks need to be uploaded before sections
316-
* 3. Liquid sections need to be uploaded afterwards
317-
* 4. JSON sections need to be uploaded after sections
318-
* 5. JSON templates need to be uploaded after all sections and layouts
319-
* 6. Contextualized templates should be uploaded after as they are variations of templates
320-
* 7. Config files must be the last ones, but we need to upload config/settings_schema.json first, followed by config/settings_data.json
316+
*
317+
* 1. config/settings_schema.json must be uploaded FIRST. It declares the
318+
* theme-level settings that block / section / section-group / template
319+
* validators resolve dynamic-source defaults against (e.g. defaults of
320+
* the form {{ settings.<theme_setting>.<property> }}). On a fresh
321+
* theme the stored schema is empty, so any later asset whose schema
322+
* references a not-yet-declared theme setting fails server-side
323+
* validation. Uploading the schema first primes those references.
324+
* 2. Layout files don't necessarily need to be the first, but they must be
325+
* uploaded before templates.
326+
* 3. Liquid blocks need to be uploaded before sections
327+
* 4. Liquid sections need to be uploaded afterwards
328+
* 5. JSON sections need to be uploaded after sections
329+
* 6. JSON templates need to be uploaded after all sections and layouts
330+
* 7. Contextualized templates should be uploaded after as they are
331+
* variations of templates
332+
* 8. config/settings_data.json must be uploaded LAST. Its current and
333+
* presets are validated against the freshly-uploaded
334+
* settings_schema.json, and presets can reference sections and
335+
* templates uploaded in earlier steps.
321336
*
322337
* The files with no dependencies we have are:
323338
* - The other Liquid files (for example, snippets, and liquid templates)
324339
* - The other JSON files (for example, locales)
325340
* - The static assets
326341
*
327342
*/
328-
function orderFilesToBeUploaded(files: ChecksumWithSize[]): {
343+
export function orderFilesToBeUploaded(files: ChecksumWithSize[]): {
329344
independentFiles: ChecksumWithSize[][]
330345
dependentFiles: ChecksumWithSize[][]
331346
} {
@@ -336,13 +351,18 @@ function orderFilesToBeUploaded(files: ChecksumWithSize[]): {
336351
independentFiles: [fileSets.otherLiquidFiles, fileSets.otherJsonFiles, fileSets.staticAssetFiles],
337352
// Follow order of dependencies:
338353
dependentFiles: [
354+
// Theme setting declarations must land before any asset that may
355+
// reference them via dynamic-source defaults. See header comment.
356+
fileSets.configSchemaFile,
339357
fileSets.layoutFiles,
340358
fileSets.blockLiquidFiles,
341359
fileSets.sectionLiquidFiles,
342360
fileSets.sectionJsonFiles,
343361
fileSets.templateJsonFiles,
344362
fileSets.contextualizedJsonFiles,
345-
fileSets.configFiles,
363+
// Settings values reference the schema we just uploaded, plus any
364+
// sections / templates referenced by presets.
365+
fileSets.configDataFile,
346366
],
347367
}
348368
}

0 commit comments

Comments
 (0)