Merge pull request #7481 from Shopify/fix/theme-push-fresh-dynamic-source-defaults

Upload settings_schema.json before validator-consumer assets

Author: NathanPJF

.changeset/fix-theme-push-fresh-dynamic-source-defaults.md

@@ -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.

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

@@ -473,7 +473,8 @@ describe('theme-fs', () => {
         otherLiquidFiles,
         templateJsonFiles,
         otherJsonFiles,
-        configFiles,
+        configSchemaFile,
+        configDataFile,
         staticAssetFiles,
         contextualizedJsonFiles,
         blockLiquidFiles,
@@ -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'},
@@ -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([])
     })
   })

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

@@ -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$/,
@@ -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[] = []
@@ -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)
@@ -506,7 +510,8 @@ export function partitionThemeFiles<T extends {key: string}>(files: T[]) {
     templateJsonFiles,
     contextualizedJsonFiles,
     otherJsonFiles,
-    configFiles,
+    configSchemaFile,
+    configDataFile,
     staticAssetFiles,
     blockLiquidFiles,
     layoutFiles,

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

@@ -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,
@@ -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',
@@ -365,7 +373,7 @@ describe('theme-uploader', () => {
     )
 
     expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
-      6,
+      7,
       remoteTheme.id,
       [
         {
@@ -376,7 +384,7 @@ describe('theme-uploader', () => {
     )
 
     expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
-      7,
+      8,
       remoteTheme.id,
       [
         {
@@ -387,7 +395,7 @@ describe('theme-uploader', () => {
     )
 
     expect(bulkUploadThemeAssets).toHaveBeenNthCalledWith(
-      8,
+      9,
       remoteTheme.id,
       [
         {
@@ -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,
     )
   })

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

@@ -213,7 +213,8 @@ function orderFilesToBeDeleted(files: Checksum[]): Checksum[] {
     ...fileSets.blockLiquidFiles,
     ...fileSets.layoutFiles,
     ...fileSets.otherLiquidFiles,
-    ...fileSets.configFiles,
+    ...fileSets.configDataFile,
+    ...fileSets.configSchemaFile,
     ...fileSets.staticAssetFiles,
   ]
 }
@@ -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)
@@ -336,13 +347,14 @@ function orderFilesToBeUploaded(files: ChecksumWithSize[]): {
     independentFiles: [fileSets.otherLiquidFiles, fileSets.otherJsonFiles, fileSets.staticAssetFiles],
     // Follow order of dependencies:
     dependentFiles: [
+      fileSets.configSchemaFile,
       fileSets.layoutFiles,
       fileSets.blockLiquidFiles,
       fileSets.sectionLiquidFiles,
       fileSets.sectionJsonFiles,
       fileSets.templateJsonFiles,
       fileSets.contextualizedJsonFiles,
-      fileSets.configFiles,
+      fileSets.configDataFile,
     ],
   }
 }