From a1e613c3aeec512b0a5c7b37e9ebe74248620370 Mon Sep 17 00:00:00 2001 From: Navdeep Date: Tue, 19 Nov 2024 19:48:22 -0500 Subject: [PATCH] Bug Fixes --- .changeset/shiny-schools-cross.md | 6 ++ .../checks/valid-block-target/block-utils.ts | 100 +++++++++++++++--- .../checks/valid-block-target/index.spec.ts | 23 ++++ .../src/checks/valid-block-target/index.ts | 14 +-- .../checks/valid-local-blocks/index.spec.ts | 38 +++++++ .../valid-local-blocks/valid-block-utils.ts | 23 +++- .../theme-check-node/configs/recommended.yml | 6 ++ 7 files changed, 187 insertions(+), 23 deletions(-) create mode 100644 .changeset/shiny-schools-cross.md diff --git a/.changeset/shiny-schools-cross.md b/.changeset/shiny-schools-cross.md new file mode 100644 index 00000000..9bb6686a --- /dev/null +++ b/.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. diff --git a/packages/theme-check-common/src/checks/valid-block-target/block-utils.ts b/packages/theme-check-common/src/checks/valid-block-target/block-utils.ts index e76f0b33..2df0a9c6 100644 --- a/packages/theme-check-common/src/checks/valid-block-target/block-utils.ts +++ b/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, 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(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, }; } diff --git a/packages/theme-check-common/src/checks/valid-block-target/index.spec.ts b/packages/theme-check-common/src/checks/valid-block-target/index.spec.ts index cf2cc124..05a7da1c 100644 --- a/packages/theme-check-common/src/checks/valid-block-target/index.spec.ts +++ b/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; + }); + }); }); diff --git a/packages/theme-check-common/src/checks/valid-block-target/index.ts b/packages/theme-check-common/src/checks/valid-block-target/index.ts index e837892b..9379e0f0 100644 --- a/packages/theme-check-common/src/checks/valid-block-target/index.ts +++ b/packages/theme-check-common/src/checks/valid-block-target/index.ts @@ -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 }, }; }, diff --git a/packages/theme-check-common/src/checks/valid-local-blocks/index.spec.ts b/packages/theme-check-common/src/checks/valid-local-blocks/index.spec.ts index f6b13932..d99c6b0b 100644 --- a/packages/theme-check-common/src/checks/valid-local-blocks/index.spec.ts +++ b/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; + }); +}); diff --git a/packages/theme-check-common/src/checks/valid-local-blocks/valid-block-utils.ts b/packages/theme-check-common/src/checks/valid-local-blocks/valid-block-utils.ts index ee8628f7..658ecaee 100644 --- a/packages/theme-check-common/src/checks/valid-local-blocks/valid-block-utils.ts +++ b/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, node: LiquidRawTag) => (location: Location) => { diff --git a/packages/theme-check-node/configs/recommended.yml b/packages/theme-check-node/configs/recommended.yml index dce67d0e..d86f3f4e 100644 --- a/packages/theme-check-node/configs/recommended.yml +++ b/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