Bug Fixes

.changeset/shiny-schools-cross.md

@@ -0,0 +1,6 @@
+---
+'@shopify/theme-check-common': minor
+'@shopify/theme-check-node': minor
+---
+
+Bug fix to support older themes.

packages/theme-check-common/src/checks/valid-block-target/block-utils.ts

@@ -2,7 +2,7 @@ import { LiquidRawTag } from '@shopify/liquid-html-parser';
 import { Context, SourceCodeType, Schema, JSONNode } from '../../types';
 import { doesFileExist } from '../../utils/file-utils';
 import { visit } from '../../visitor';
-import { LiteralNode } from 'json-to-ast';
+import { LiteralNode, PropertyNode } from 'json-to-ast';
 
 type BlockTypeMap = { [key: string]: Location[] };
 
@@ -11,16 +11,31 @@ type Location = {
   endIndex: number;
 };
 
+type NestedBlockLocation = {
+  location: Location;
+  parentBlockTypes: string[];
+};
+
+type NestedPresetBlockMap = {
+  [key: string]: NestedBlockLocation[];
+};
+
 type BlockValidationResult = {
-  rootBlockTypes: BlockTypeMap;
+  hasLocalBlocks: boolean;
+  rootThemeBlockTypes: BlockTypeMap;
   presetBlockTypes: BlockTypeMap;
+  nestedPresetBlockTypes: NestedPresetBlockMap;
 };
 
 function isLiteralNode(node: JSONNode): node is LiteralNode {
   return node.type === 'Literal';
 }
 
-// Function to determine if a node is in an array with a specific parent key
+function isPropertyNode(node: JSONNode): node is PropertyNode {
+  return node.type === 'Property';
+}
+
+// Check if a node is within an array that has a specific parent key
 function isInArrayWithParentKey(ancestors: JSONNode[], parentKey: string): boolean {
   return ancestors.some((ancestor, index) => {
     const parent = ancestors[index - 1];
@@ -32,6 +47,26 @@ function isInArrayWithParentKey(ancestors: JSONNode[], parentKey: string): boole
   });
 }
 
+function isInBlocksArray(ancestors: JSONNode[]): boolean {
+  const thirdAncestor = ancestors[ancestors.length - 3];
+  const fourthAncestor = ancestors[ancestors.length - 4];
+
+  return (
+    (isPropertyNode(thirdAncestor) && thirdAncestor.key?.value === 'blocks') ||
+    (isPropertyNode(fourthAncestor) && fourthAncestor.key?.value === 'blocks')
+  );
+}
+
+function isInPresetsArray(ancestors: JSONNode[]): boolean {
+  const sixthAncestor = ancestors[ancestors.length - 6];
+  const seventhAncestor = ancestors[ancestors.length - 7];
+
+  return (
+    (isPropertyNode(sixthAncestor) && sixthAncestor.key?.value === 'presets') ||
+    (isPropertyNode(seventhAncestor) && seventhAncestor.key?.value === 'presets')
+  );
+}
+
 export const reportError =
   (message: string, context: Context<SourceCodeType.LiquidHtml, Schema>, node: LiquidRawTag) =>
   (location: Location) => {
@@ -42,36 +77,75 @@ export const reportError =
     });
   };
 
+function getParentBlockTypes(ancestors: JSONNode[]): string[] {
+  // TODO: Implement this
+  return [];
+}
+
 export function collectAndValidateBlockTypes(jsonFile: JSONNode): BlockValidationResult {
-  const rootBlockTypes: BlockTypeMap = {};
+  const rootThemeBlockTypes: BlockTypeMap = {};
+  const rootLocalBlockTypes: BlockTypeMap = {};
   const presetBlockTypes: BlockTypeMap = {};
-
+  const nestedPresetBlockTypes: NestedPresetBlockMap = {};
   visit<SourceCodeType.JSON, void>(jsonFile, {
     Property(node, ancestors) {
-      // Only process type and name properties within blocks
+      // Process 'type' and 'name' properties within 'blocks'
       if (!isInArrayWithParentKey(ancestors, 'blocks') || !isLiteralNode(node.value)) return;
 
-      if (node.key.value === 'type') {
+      const parentObject = ancestors[ancestors.length - 1];
+      const hasNameProperty =
+        parentObject.type === 'Object' &&
+        parentObject.children.some(
+          (child) => child.type === 'Property' && child.key.value === 'name',
+        );
+
+      if (node.key.value === 'type' && isInBlocksArray(ancestors)) {
         const typeValue = node.value.value;
         const typeLocation = {
           startIndex: node.value.loc!.start.offset,
           endIndex: node.value.loc!.end.offset,
         };
 
-        // Add to appropriate map
+        // Determine the target map for block types based on their context
+        type TargetMapType = BlockTypeMap | NestedPresetBlockMap | undefined;
+
+        // Determine the target map for block types based on their context
+        let targetMap: TargetMapType;
         const inPresets = isInArrayWithParentKey(ancestors, 'presets');
-        const targetMap = inPresets ? presetBlockTypes : rootBlockTypes;
-        if (typeof typeValue === 'string') {
-          targetMap[typeValue] = targetMap[typeValue] || [];
-          targetMap[typeValue].push(typeLocation);
+
+        if (inPresets && !isInPresetsArray(ancestors)) {
+          targetMap = nestedPresetBlockTypes;
+        } else if (inPresets && isInPresetsArray(ancestors)) {
+          targetMap = presetBlockTypes;
+        } else if (hasNameProperty) {
+          targetMap = rootLocalBlockTypes;
+        } else {
+          targetMap = rootThemeBlockTypes;
+        }
+
+        // Add the block type to the appropriate map
+        if (targetMap && typeof typeValue === 'string') {
+          if (targetMap === nestedPresetBlockTypes) {
+            const parentTypes = getParentBlockTypes(ancestors);
+            targetMap[typeValue] = targetMap[typeValue] || [];
+            targetMap[typeValue].push({
+              location: typeLocation,
+              parentBlockTypes: parentTypes,
+            });
+          } else {
+            (targetMap as BlockTypeMap)[typeValue] = (targetMap as BlockTypeMap)[typeValue] || [];
+            (targetMap as BlockTypeMap)[typeValue].push(typeLocation);
+          }
         }
       }
     },
   });
 
   return {
-    rootBlockTypes,
+    hasLocalBlocks: Object.keys(rootLocalBlockTypes).length > 0,
+    rootThemeBlockTypes,
     presetBlockTypes,
+    nestedPresetBlockTypes,
   };
 }
 

packages/theme-check-common/src/checks/valid-block-target/index.spec.ts

@@ -481,4 +481,27 @@ describe('Module: ValidBlockTarget', () => {
       });
     });
   });
+
+  describe('Local Block Targeting Tests', () => {
+    it('should not report errors for locally scoped blocks at root level', async () => {
+      const theme: MockTheme = {
+        'sections/local-blocks.liquid': `
+          {% schema %}
+          {
+            "name": "Section name",
+            "blocks": [
+              {
+                "type": "local_block",
+                "name": "Local block"
+              }
+            ]
+          }
+          {% endschema %}
+        `,
+      };
+
+      const offenses = await check(theme, [ValidBlockTarget]);
+      expect(offenses).to.be.empty;
+    });
+  });
 });

packages/theme-check-common/src/checks/valid-block-target/index.ts

@@ -18,7 +18,7 @@ export const ValidBlockTarget: LiquidCheckDefinition = {
     docs: {
       description:
         'Ensures block types only reference valid block types and respect parent-child relationships',
-      recommended: false,
+      recommended: true,
       url: 'https://shopify.dev/docs/themes/tools/theme-check/checks/valid-block-target',
     },
     type: SourceCodeType.LiquidHtml,
@@ -42,12 +42,12 @@ export const ValidBlockTarget: LiquidCheckDefinition = {
         const jsonFile = toJSONAST(jsonString);
         if (jsonFile instanceof Error) return;
 
-        const { rootBlockTypes, presetBlockTypes } = collectAndValidateBlockTypes(
-          jsonFile as JSONNode,
-        );
+        const { hasLocalBlocks, rootThemeBlockTypes, presetBlockTypes, nestedPresetBlockTypes } =
+          collectAndValidateBlockTypes(jsonFile as JSONNode);
 
+        if (hasLocalBlocks) return;
         let errorsInRootLevelBlocks = false;
-        for (const [blockType, locations] of Object.entries(rootBlockTypes)) {
+        for (const [blockType, locations] of Object.entries(rootThemeBlockTypes)) {
           const exists = await validateBlockFileExistence(blockType, context);
           if (!exists) {
             locations.forEach(
@@ -65,8 +65,8 @@ export const ValidBlockTarget: LiquidCheckDefinition = {
 
         for (const [presetType, locations] of Object.entries(presetBlockTypes)) {
           const isPrivateBlockType = presetType.startsWith('_');
-          const isPresetInRootLevel = presetType in rootBlockTypes;
-          const isThemeInRootLevel = '@theme' in rootBlockTypes;
+          const isPresetInRootLevel = presetType in rootThemeBlockTypes;
+          const isThemeInRootLevel = '@theme' in rootThemeBlockTypes;
           const needsExplicitRootBlock = isPrivateBlockType || !isThemeInRootLevel;
 
           if (!isPresetInRootLevel && needsExplicitRootBlock) {
@@ -88,6 +88,8 @@ export const ValidBlockTarget: LiquidCheckDefinition = {
             }
           }
         }
+
+        // TODO: Validate nested preset block types via cross file check
       },
     };
   },

packages/theme-check-common/src/checks/valid-local-blocks/index.spec.ts

@@ -329,3 +329,41 @@ describe('ValidLocalBlocks with hash-style presets', () => {
     expect(offenses[0].message).to.equal('Static theme blocks cannot have a name property.');
   });
 });
+
+describe('ValidLocalBlocks only reports errors for blocks', () => {
+  it('should not report errors for block setting types', async () => {
+    const theme: MockTheme = {
+      'sections/local-blocks.liquid': `
+        {% schema %}
+        {
+          "name": "Section name",
+          "blocks": [
+            {
+              "type": "@app"
+            },
+            {
+              "type": "link_list",
+              "name": "Link list",
+              "settings": [
+                {
+                  "type": "inline_richtext",
+                  "id": "heading",
+                  "default": "Heading"
+                },
+                {
+                  "type": "link_list",
+                  "id": "menu",
+                  "default": "Footer"
+                }
+              ]
+            }
+          ]
+        }
+        {% endschema %}
+      `,
+    };
+
+    const offenses = await check(theme, [ValidLocalBlocks]);
+    expect(offenses).to.be.empty;
+  });
+});

packages/theme-check-common/src/checks/valid-local-blocks/index.ts

@@ -14,7 +14,7 @@ export const ValidLocalBlocks: LiquidCheckDefinition = {
     docs: {
       description:
         'Ensures sections without theme block support do not mix static and local blocks',
-      recommended: false,
+      recommended: true,
       url: 'https://shopify.dev/docs/themes/tools/theme-check/checks/valid-local-blocks',
     },
     type: SourceCodeType.LiquidHtml,

packages/theme-check-common/src/checks/valid-local-blocks/valid-block-utils.ts

@@ -1,7 +1,7 @@
 import { LiquidRawTag } from '@shopify/liquid-html-parser';
 import { Context, SourceCodeType, Schema, JSONNode } from '../../types';
 import { visit } from '../../visitor';
-import { LiteralNode } from 'json-to-ast';
+import { LiteralNode, PropertyNode } from 'json-to-ast';
 
 type Location = {
   startIndex: number;
@@ -12,6 +12,10 @@ function isLiteralNode(node: JSONNode): node is LiteralNode {
   return node.type === 'Literal';
 }
 
+function isPropertyNode(node: JSONNode): node is PropertyNode {
+  return node.type === 'Property';
+}
+
 export function collectBlockProperties(jsonFile: JSONNode): {
   hasLocalBlocks: boolean;
   hasStaticBlocks: boolean;
@@ -37,7 +41,7 @@ export function collectBlockProperties(jsonFile: JSONNode): {
           (child) => child.type === 'Property' && child.key.value === 'static',
         );
 
-      if (node.key.value === 'type') {
+      if (node.key.value === 'type' && isInBlocksArray(ancestors)) {
         const typeValue = node.value.value;
         const typeLocation = {
           startIndex: node.value.loc!.start.offset,
@@ -55,11 +59,12 @@ export function collectBlockProperties(jsonFile: JSONNode): {
         } else if (
           !hasName &&
           typeValue !== '@app' &&
-          !isInArrayWithParentKey(ancestors, 'presets')
+          !isInArrayWithParentKey(ancestors, 'presets') &&
+          !isInArrayWithParentKey(ancestors, 'default')
         ) {
           themeBlockLocations.push(typeLocation);
         }
-      } else if (node.key.value === 'name') {
+      } else if (node.key.value === 'name' && isInBlocksArray(ancestors)) {
         const nameKeyLocation = {
           startIndex: node.key.loc!.start.offset,
           endIndex: node.key.loc!.end.offset,
@@ -95,6 +100,16 @@ function isInArrayWithParentKey(ancestors: JSONNode[], parentKey: string): boole
   });
 }
 
+function isInBlocksArray(ancestors: JSONNode[]): boolean {
+  const thirdAncestor = ancestors[ancestors.length - 3];
+  const fourthAncestor = ancestors[ancestors.length - 4];
+
+  return (
+    (isPropertyNode(thirdAncestor) && thirdAncestor.key?.value === 'blocks') ||
+    (isPropertyNode(fourthAncestor) && fourthAncestor.key?.value === 'blocks')
+  );
+}
+
 export const reportError =
   (message: string, context: Context<SourceCodeType.LiquidHtml, Schema>, node: LiquidRawTag) =>
   (location: Location) => {

packages/theme-check-node/configs/recommended.yml

@@ -81,6 +81,9 @@ UnknownFilter:
 UnusedAssign:
   enabled: true
   severity: 1
+ValidBlockTarget:
+  enabled: true
+  severity: 0
 ValidContentForArguments:
   enabled: true
   severity: 0
@@ -90,6 +93,9 @@ ValidHTMLTranslation:
 ValidJSON:
   enabled: true
   severity: 0
+ValidLocalBlocks:
+  enabled: true
+  severity: 0
 ValidSchema:
   enabled: true
   severity: 0