Skip to content

Upload settings_schema.json before validator-consumer assets #7481

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
NathanPJF merged 4 commits into from
May 8, 2026
Merged

Upload settings_schema.json before validator-consumer assets #7481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1e8963e
Upload settings_schema.json before validator-consumer assets
NathanPJF May 6, 2026
aefc063
Update changeset to reference color_palette
NathanPJF May 6, 2026
bfe4940
Remove redundant order test
NathanPJF May 7, 2026
fad7904
PR review feedback
NathanPJF May 8, 2026
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/fix-theme-push-fresh-dynamic-source-defaults.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/theme': patch
---

Upload `config/settings_schema.json` before any other theme file. Fixes `theme push` failing on the first push when blocks or sections reference a `color_palette` theme setting.
23 changes: 15 additions & 8 deletions packages/theme/src/cli/utilities/theme-fs.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,8 @@ describe('theme-fs', () => {
otherLiquidFiles,
templateJsonFiles,
otherJsonFiles,
configFiles,
configSchemaFile,
configDataFile,
staticAssetFiles,
contextualizedJsonFiles,
blockLiquidFiles,
Expand All @@ -489,10 +490,8 @@ describe('theme-fs', () => {
])
expect(otherJsonFiles).toEqual([{key: 'locales/en.default.json', checksum: '6'}])
expect(templateJsonFiles).toEqual([{key: 'templates/404.json', checksum: '7'}])
expect(configFiles).toEqual([
{key: 'config/settings_schema.json', checksum: '8'},
{key: 'config/settings_data.json', checksum: '9'},
])
expect(configSchemaFile).toEqual([{key: 'config/settings_schema.json', checksum: '8'}])
expect(configDataFile).toEqual([{key: 'config/settings_data.json', checksum: '9'}])
expect(staticAssetFiles).toEqual([
{key: 'assets/base.css', checksum: '1'},
{key: 'assets/sparkle.gif', checksum: '3'},
Expand All @@ -511,15 +510,23 @@ describe('theme-fs', () => {
const files: Checksum[] = []

// When
const {sectionLiquidFiles, otherLiquidFiles, templateJsonFiles, otherJsonFiles, configFiles, staticAssetFiles} =
partitionThemeFiles(files)
const {
sectionLiquidFiles,
otherLiquidFiles,
templateJsonFiles,
otherJsonFiles,
configSchemaFile,
configDataFile,
staticAssetFiles,
} = partitionThemeFiles(files)

// Then
expect(sectionLiquidFiles).toEqual([])
expect(otherLiquidFiles).toEqual([])
expect(templateJsonFiles).toEqual([])
expect(otherJsonFiles).toEqual([])
expect(configFiles).toEqual([])
expect(configSchemaFile).toEqual([])
expect(configDataFile).toEqual([])
expect(staticAssetFiles).toEqual([])
})
})
Expand Down
15 changes: 10 additions & 5 deletions packages/theme/src/cli/utilities/theme-fs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ const THEME_PARTITION_REGEX = {
layoutLiquidRegex: /^layout\/.+\.liquid$/,
sectionLiquidRegex: /^sections\/.+\.liquid$/,
blockLiquidRegex: /^blocks\/.+\.liquid$/,
configRegex: /^config\/(settings_schema|settings_data)\.json$/,
configSchemaRegex: /^config\/settings_schema\.json$/,
configDataRegex: /^config\/settings_data\.json$/,
sectionJsonRegex: /^sections\/.+\.json$/,
templateJsonRegex: /^templates\/.+\.json$/,
jsonRegex: /^(?!config\/).*\.json$/,
Expand Down Expand Up @@ -465,7 +466,8 @@ export function partitionThemeFiles<T extends {key: string}>(files: T[]) {
const templateJsonFiles: T[] = []
const otherJsonFiles: T[] = []
const contextualizedJsonFiles: T[] = []
const configFiles: T[] = []
const configSchemaFile: T[] = []
const configDataFile: T[] = []
const staticAssetFiles: T[] = []
const blockLiquidFiles: T[] = []
const layoutFiles: T[] = []
Expand All @@ -482,8 +484,10 @@ export function partitionThemeFiles<T extends {key: string}>(files: T[]) {
} else {
otherLiquidFiles.push(file)
}
} else if (THEME_PARTITION_REGEX.configRegex.test(fileKey)) {
configFiles.push(file)
} else if (THEME_PARTITION_REGEX.configSchemaRegex.test(fileKey)) {
configSchemaFile.push(file)
} else if (THEME_PARTITION_REGEX.configDataRegex.test(fileKey)) {
configDataFile.push(file)
} else if (THEME_PARTITION_REGEX.jsonRegex.test(fileKey)) {
if (THEME_PARTITION_REGEX.contextualizedJsonRegex.test(fileKey)) {
contextualizedJsonFiles.push(file)
Expand All @@ -506,7 +510,8 @@ export function partitionThemeFiles<T extends {key: string}>(files: T[]) {
templateJsonFiles,
contextualizedJsonFiles,
otherJsonFiles,
configFiles,
configSchemaFile,
configDataFile,
staticAssetFiles,
blockLiquidFiles,
layoutFiles,
Expand Down
38 changes: 20 additions & 18 deletions packages/theme/src/cli/utilities/theme-uploader.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,17 +319,19 @@ describe('theme-uploader', () => {
await renderThemeSyncProgress()

// Then
expect(bulkUploadThemeAssets).toHaveBeenCalledTimes(9)
expect(bulkUploadThemeAssets).toHaveBeenCalledTimes(10)
// Minimum theme files start first
expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(1, remoteTheme.id, MINIMUM_THEME_ASSETS, adminSession)
// Dependent assets start second
// settings_schema.json is uploaded first among dependent files so block / section /
// section-group / template validators can resolve dynamic-source defaults that
// reference theme-level settings declared in the schema.
expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
2,
remoteTheme.id,
[{key: 'layout/theme.liquid'}],
[{key: 'config/settings_schema.json'}],
adminSession,
)
// Independent assets start right after dependent assets start
// Independent assets fan out concurrently with the dependent chain.
expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
3,
remoteTheme.id,
Expand All @@ -346,16 +348,22 @@ describe('theme-uploader', () => {
],
adminSession,
)
// Dependent assets continue after the first batch of dependent assets ends
// Layout files come after the schema is in place.
expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
4,
remoteTheme.id,
[{key: 'blocks/block.liquid'}],
[{key: 'layout/theme.liquid'}],
adminSession,
)
expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
5,
remoteTheme.id,
[{key: 'blocks/block.liquid'}],
adminSession,
)
expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
6,
remoteTheme.id,
[
{
key: 'sections/header.liquid',
Expand All @@ -365,7 +373,7 @@ describe('theme-uploader', () => {
)

expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
6,
7,
remoteTheme.id,
[
{
Expand All @@ -376,7 +384,7 @@ describe('theme-uploader', () => {
)

expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
7,
8,
remoteTheme.id,
[
{
Expand All @@ -387,7 +395,7 @@ describe('theme-uploader', () => {
)

expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
8,
9,
remoteTheme.id,
[
{
Expand All @@ -397,17 +405,11 @@ describe('theme-uploader', () => {
adminSession,
)

// settings_data.json must be last
expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
9,
10,
remoteTheme.id,
[
{
key: 'config/settings_data.json',
},
{
key: 'config/settings_schema.json',
},
],
[{key: 'config/settings_data.json'}],
adminSession,
)
})
Expand Down
30 changes: 21 additions & 9 deletions packages/theme/src/cli/utilities/theme-uploader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,8 @@ function orderFilesToBeDeleted(files: Checksum[]): Checksum[] {
...fileSets.blockLiquidFiles,
...fileSets.layoutFiles,
...fileSets.otherLiquidFiles,
...fileSets.configFiles,
...fileSets.configDataFile,
...fileSets.configSchemaFile,
Copy link
Copy Markdown
Contributor

@dmerand dmerand May 8, 2026

Choose a reason for hiding this comment

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

Does this ordering matter at runtime for deletion as well? Currently it looks like deletion happens in async batches where the ordering might not be guaranteed.

Copy link
Copy Markdown
Contributor Author

@NathanPJF NathanPJF May 8, 2026

Choose a reason for hiding this comment

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

Effectively it doesn't. My understanding is that mirrors the upload just for consistency, but not an actual contract. So I'll update my inline comment about "config file consideration" just to not make it seem important

But as to whether it actually matters: the utility of deleting is getting the remote theme in sync with local dev. Doesn't matter where settings_schema is because there's no use-case for ever deleting that file and keeping a functioning theme.

...fileSets.staticAssetFiles,
]
}
Expand Down Expand Up @@ -311,13 +312,23 @@ function selectUploadableFiles(themeFileSystem: ThemeFileSystem, remoteChecksums
* We use this 2d array to batch files of the same type together
* while maintaining the order between file types. The files with
* dependencies we have are:
* 1. Layout files don't necessarily need to be the first, but they must uploaded before templates.
* 2. Liquid blocks need to be uploaded before sections
* 3. Liquid sections need to be uploaded afterwards
* 4. JSON sections need to be uploaded after sections
* 5. JSON templates need to be uploaded after all sections and layouts
* 6. Contextualized templates should be uploaded after as they are variations of templates
* 7. Config files must be the last ones, but we need to upload config/settings_schema.json first, followed by config/settings_data.json
*
* 1. config/settings_schema.json must be uploaded FIRST. It declares the
* theme-level settings that block / section / section-group / template
* validators resolve dynamic-source defaults against (e.g. defaults of
* the form `{{ settings.<theme_setting>.<property> }}`).
* 2. Layout files don't necessarily need to be the first, but they must be
* uploaded before templates.
* 3. Liquid blocks need to be uploaded before sections
* 4. Liquid sections need to be uploaded afterwards
* 5. JSON sections need to be uploaded after sections
* 6. JSON templates need to be uploaded after all sections and layouts
* 7. Contextualized templates should be uploaded after as they are
* variations of templates
* 8. config/settings_data.json must be uploaded LAST. Its current and
* presets are validated against the freshly-uploaded
* settings_schema.json, and presets can reference sections and
* templates uploaded in earlier steps.
*
* The files with no dependencies we have are:
* - The other Liquid files (for example, snippets, and liquid templates)
Expand All @@ -336,13 +347,14 @@ function orderFilesToBeUploaded(files: ChecksumWithSize[]): {
independentFiles: [fileSets.otherLiquidFiles, fileSets.otherJsonFiles, fileSets.staticAssetFiles],
// Follow order of dependencies:
dependentFiles: [
fileSets.configSchemaFile,
Copy link
Copy Markdown
Contributor

@dmerand dmerand May 8, 2026

Choose a reason for hiding this comment

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

If schema upload fails, would you want later dependent uploads to proceed? Current batch upload doesn't throw when bulk upload fails.

Copy link
Copy Markdown
Contributor Author

@NathanPJF NathanPJF May 8, 2026

Choose a reason for hiding this comment

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

This is a great point, but I don't want to tackle that in this PR because its a larger change to the failure mode of theme push. Today theme uploads behave as "upload as much as possible" and we get reports of what couldn't be uploaded. Adding a dependency check sounds useful, it's a "fail fast" approach - something that we could be benefitting from already given the dependencies for Layout files > JSON templates, Liquid sections > JSON sections, etc. But I'm hesitant to change it just yet as I'm no sure how devs may be benefitting from the current "upload as much as possible" mechanic.

fileSets.layoutFiles,
fileSets.blockLiquidFiles,
fileSets.sectionLiquidFiles,
fileSets.sectionJsonFiles,
fileSets.templateJsonFiles,
fileSets.contextualizedJsonFiles,
fileSets.configFiles,
fileSets.configDataFile,
],
}
}
Expand Down
Loading