From 99c251282b84027affe28abadcf8e6962a883ed0 Mon Sep 17 00:00:00 2001 From: Ariba Rajput Date: Thu, 2 Jan 2025 10:59:27 -0500 Subject: [PATCH] Added in a check on script tags in theme app extensions to make sure importmap is not being used --- .changeset/chilled-bugs-juggle.md | 5 - .changeset/yellow-insects-draw.md | 5 + .../theme-check-common/src/checks/index.ts | 4 +- .../src/checks/no-import-map/index.spec.ts | 31 ++++ .../src/checks/no-import-map/index.ts | 54 ++++++ .../valid-block-preset-settings/index.spec.ts | 166 ------------------ .../valid-block-preset-settings/index.ts | 114 ------------ packages/theme-check-node/configs/all.yml | 6 +- .../theme-check-node/configs/recommended.yml | 3 - .../configs/theme-app-extension.yml | 3 + 10 files changed, 98 insertions(+), 293 deletions(-) delete mode 100644 .changeset/chilled-bugs-juggle.md create mode 100644 .changeset/yellow-insects-draw.md create mode 100644 packages/theme-check-common/src/checks/no-import-map/index.spec.ts create mode 100644 packages/theme-check-common/src/checks/no-import-map/index.ts delete mode 100644 packages/theme-check-common/src/checks/valid-block-preset-settings/index.spec.ts delete mode 100644 packages/theme-check-common/src/checks/valid-block-preset-settings/index.ts diff --git a/.changeset/chilled-bugs-juggle.md b/.changeset/chilled-bugs-juggle.md deleted file mode 100644 index 61a1a8bf7..000000000 --- a/.changeset/chilled-bugs-juggle.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'@shopify/theme-check-common': minor ---- - -Added theme check for preset settings validation on theme blocks diff --git a/.changeset/yellow-insects-draw.md b/.changeset/yellow-insects-draw.md new file mode 100644 index 000000000..4c1730c1b --- /dev/null +++ b/.changeset/yellow-insects-draw.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme-check-common': minor +--- + +This theme check will check for and suggest removing script tags with type importmap on theme app extensions diff --git a/packages/theme-check-common/src/checks/index.ts b/packages/theme-check-common/src/checks/index.ts index e0803940a..92b4b9d9b 100644 --- a/packages/theme-check-common/src/checks/index.ts +++ b/packages/theme-check-common/src/checks/index.ts @@ -45,7 +45,7 @@ import { ValidSchemaName } from './valid-schema-name'; import { ValidStaticBlockType } from './valid-static-block-type'; import { VariableName } from './variable-name'; import { MissingSchema } from './missing-schema'; -import { ValidBlockPresetSettings } from './valid-block-preset-settings'; +import { NoImportMap } from './no-import-map'; export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [ AppBlockValidTags, @@ -93,7 +93,7 @@ export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [ ValidStaticBlockType, VariableName, ValidSchemaName, - ValidBlockPresetSettings, + NoImportMap, ]; /** diff --git a/packages/theme-check-common/src/checks/no-import-map/index.spec.ts b/packages/theme-check-common/src/checks/no-import-map/index.spec.ts new file mode 100644 index 000000000..cf83ec57b --- /dev/null +++ b/packages/theme-check-common/src/checks/no-import-map/index.spec.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from 'vitest'; +import { NoImportMap } from '.'; +import { check as reportOffenses } from '../../test'; + +describe('Module: NoImportMap', () => { + it('should report offense when using `; + const startIndex = file.indexOf('') + ''.length; + + const offenses = await reportOffenses({ 'code.liquid': file }, [NoImportMap]); + + expect(offenses).to.have.length(1); + const { message, start, end } = offenses[0]; + + expect(message).toEqual( + 'Until browsers permit multiple importmap entries, only themes can have an importmap', + ); + expect(start.index).toEqual(startIndex); + expect(end.index).toEqual(endIndex); + + expect(offenses[0].suggest).to.have.length(1); + expect(offenses[0]!.suggest![0].message).to.equal("Remove the 'importmap' script tag"); + }); +}); diff --git a/packages/theme-check-common/src/checks/no-import-map/index.ts b/packages/theme-check-common/src/checks/no-import-map/index.ts new file mode 100644 index 000000000..d5c913e4c --- /dev/null +++ b/packages/theme-check-common/src/checks/no-import-map/index.ts @@ -0,0 +1,54 @@ +import { ConfigTarget, LiquidCheckDefinition, Severity, SourceCodeType } from '../../types'; + +import { hasAttributeValueOf, isAttr, isValuedHtmlAttribute } from '../utils'; + +export const NoImportMap: LiquidCheckDefinition = { + meta: { + code: 'NoImportMap', + name: 'Import map in theme app extensions', + docs: { + description: + 'Report offenses associated with import maps on script tags in theme app extensions', + url: 'https://shopify.dev/docs/storefronts/themes/tools/theme-check/checks/no-import-map', + }, + type: SourceCodeType.LiquidHtml, + severity: Severity.ERROR, + schema: {}, + targets: [ConfigTarget.ThemeAppExtension], + }, + + create(context) { + return { + async HtmlRawNode(node) { + if (node.name !== 'script') { + return; + } + + const typeImportMap = node.attributes + .filter(isValuedHtmlAttribute) + .some((attr) => isAttr(attr, 'type') && hasAttributeValueOf(attr, 'importmap')); + + const typeModule = node.attributes + .filter(isValuedHtmlAttribute) + .some((attr) => isAttr(attr, 'type') && hasAttributeValueOf(attr, 'importmap')); + + if (!typeImportMap || !typeModule) { + return; + } + + context.report({ + message: + 'Until browsers permit multiple importmap entries, only themes can have an importmap', + startIndex: node.position.start, + endIndex: node.position.end, + suggest: [ + { + message: `Remove the 'importmap' script tag`, + fix: (corrector) => corrector.remove(node.position.start, node.position.end), + }, + ], + }); + }, + }; + }, +}; diff --git a/packages/theme-check-common/src/checks/valid-block-preset-settings/index.spec.ts b/packages/theme-check-common/src/checks/valid-block-preset-settings/index.spec.ts deleted file mode 100644 index 944dee033..000000000 --- a/packages/theme-check-common/src/checks/valid-block-preset-settings/index.spec.ts +++ /dev/null @@ -1,166 +0,0 @@ -import { describe, expect, it } from 'vitest'; -import { ValidBlockPresetSettings } from '.'; -import { check } from '../../test/test-helper'; -import { MockTheme } from '../../test/MockTheme'; - -describe('ValidBlockPresetSettings', () => { - it('should report invalid preset settings', async () => { - const theme: MockTheme = { - 'blocks/price.liquid': ` - {% schema %} - { - "name": "t:names.product_price", - "settings": [ - { - "type": "product", - "id": "product", - "label": "t:settings.product" - }, - ], - "presets": [ - { - "name": "t:names.product_price", - "settings": { - "product": "{{ context.product }}", - "undefined_setting": "some value", - } - } - ] - } - {% endschema %} - `, - }; - const offenses = await check(theme, [ValidBlockPresetSettings]); - expect(offenses).to.have.length(1); - }); - - it('should report invalid theme block preset settings', async () => { - const theme: MockTheme = { - 'blocks/block_1.liquid': ` - {% schema %} - { - "name": "t:names.block_1", - "settings": [ - { - "type": "text", - "id": "block_1_setting_key", - "label": "t:settings.block_1" - }, - ] - } - {% endschema %} - `, - 'blocks/price.liquid': ` - {% schema %} - { - "name": "t:names.product_price", - "settings": [ - { - "type": "product", - "id": "product", - "label": "t:settings.product" - } - ], - "blocks": [ - { - "type": "block_1", - "name": "t:names.block_1", - } - ], - "presets": [ - { - "name": "t:names.product_price", - "settings": { - "product": "{{ context.product }}", - }, - "blocks": [ - { - "block_1": { - "type": "block_1", - "settings": { - "block_1_setting_key": "correct setting key", - "undefined_setting": "incorrect setting key" - } - } - } - ], - } - ] - } - {% endschema %} - `, - }; - - const offenses = await check(theme, [ValidBlockPresetSettings]); - expect(offenses).to.have.length(1); - expect(offenses[0].message).to.include( - 'Preset setting "undefined_setting" does not exist in the block type "block_1"\'s settings', - ); - }); - - it('should not report when all section and block preset settings are valid', async () => { - const theme: MockTheme = { - 'blocks/block_1.liquid': ` - {% schema %} - { - "name": "t:names.block_1", - "settings": [ - { - "type": "text", - "id": "block_1_setting_key", - "label": "t:settings.block_1" - } - ] - } - {% endschema %} - `, - 'blocks/price.liquid': ` - {% schema %} - { - "name": "t:names.product_price", - "settings": [ - { - "type": "product", - "id": "product", - "label": "t:settings.product" - }, - { - "type": "text", - "id": "section_setting", - "label": "t:settings.section" - } - ], - "blocks": [ - { - "type": "block_1", - "name": "t:names.block_1" - } - ], - "presets": [ - { - "name": "t:names.product_price", - "settings": { - "product": "{{ context.product }}", - "section_setting": "some value" - }, - "blocks": [ - { - "block_1": { - "type": "block_1", - "settings": { - "block_1_setting_key": "correct setting key" - } - } - } - ] - } - ] - } - {% endschema %} - `, - }; - - const offenses = await check(theme, [ValidBlockPresetSettings]); - expect(offenses).to.have.length(0); - }); -}); diff --git a/packages/theme-check-common/src/checks/valid-block-preset-settings/index.ts b/packages/theme-check-common/src/checks/valid-block-preset-settings/index.ts deleted file mode 100644 index 88a50728e..000000000 --- a/packages/theme-check-common/src/checks/valid-block-preset-settings/index.ts +++ /dev/null @@ -1,114 +0,0 @@ -//we want to check if the preset key exists in the settings - -import { isSection } from '../../to-schema'; -import { basename } from '../../path'; -import { isBlock } from '../../to-schema'; -import { LiquidCheckDefinition, Preset, Severity, SourceCodeType, ThemeBlock } from '../../types'; - -export const ValidBlockPresetSettings: LiquidCheckDefinition = { - meta: { - code: 'ValidBlockPresetSettings', - name: 'Reports invalid preset settings for a theme block', - docs: { - description: 'Reports invalid preset settings for a theme block', - recommended: true, - url: 'https://shopify.dev/docs/storefronts/themes/tools/theme-check/checks/preset-key-exists', - }, - severity: Severity.ERROR, - type: SourceCodeType.LiquidHtml, - schema: {}, - targets: [], - }, - - create(context) { - function getSchema() { - const name = basename(context.file.uri, '.liquid'); - switch (true) { - case isBlock(context.file.uri): - return context.getBlockSchema?.(name); - case isSection(context.file.uri): - return context.getSectionSchema?.(name); - default: - return undefined; - } - } - - function getInlineSettingsTypesandKeys(settings: ThemeBlock.Schema['settings']) { - if (!settings) return []; - return settings.map((setting: { id: any; type: any }) => ({ - id: setting.id, - type: setting.type, - })); - } - - function getPresetSettingsKeys(presets: Preset.Preset[]) { - const allKeys: string[] = []; - for (const preset of presets) { - if (preset.settings) { - allKeys.push(...Object.keys(preset.settings)); - } - } - return allKeys; - } - - function getPresetBlockSettingsKeys(blocks: Preset.PresetBlocks) { - const allKeys: string[] = []; - for (const block of Object.values(blocks)) { - for (const [_, blockData] of Object.entries(block)) { - if (blockData && typeof blockData === 'object' && 'settings' in blockData) { - allKeys.push(...Object.keys(blockData.settings)); - } - } - return allKeys; - } - } - - return { - async LiquidRawTag(node) { - if (node.name !== 'schema' || node.body.kind !== 'json') { - return; - } - - const schema = await getSchema(); - if (!schema) return; - if (schema.validSchema instanceof Error) return; - - const validSchema = schema.validSchema; - const settingsKeys = getInlineSettingsTypesandKeys(validSchema.settings); - const presetSettingsKeys = getPresetSettingsKeys(validSchema.presets ?? []); - - for (const key of presetSettingsKeys) { - if (!settingsKeys.some((setting) => setting.id === key)) { - context.report({ - message: `Preset setting "${key}" does not exist in the block's settings`, - startIndex: 0, - endIndex: 0, - }); - } - } - - if (validSchema.blocks) { - for (const block of validSchema.blocks) { - const blockSchema = await context.getBlockSchema?.(block.type); - if (!blockSchema || blockSchema.validSchema instanceof Error) continue; - - for (const preset of validSchema.presets ?? []) { - if (!preset.blocks) continue; - const presetBlockSettingKeys = getPresetBlockSettingsKeys(preset.blocks) ?? []; - - for (const key of presetBlockSettingKeys) { - if (!blockSchema.validSchema.settings?.some((setting) => setting.id === key)) { - context.report({ - message: `Preset setting "${key}" does not exist in the block type "${block.type}"'s settings`, - startIndex: 0, - endIndex: 0, - }); - } - } - } - } - } - }, - }; - }, -}; diff --git a/packages/theme-check-node/configs/all.yml b/packages/theme-check-node/configs/all.yml index e9f80d36b..3f692d247 100644 --- a/packages/theme-check-node/configs/all.yml +++ b/packages/theme-check-node/configs/all.yml @@ -80,6 +80,9 @@ MissingTemplate: enabled: true severity: 0 ignoreMissing: [] +NoImportMap: + enabled: false + severity: 0 PaginationSize: enabled: true severity: 1 @@ -118,9 +121,6 @@ UnknownFilter: UnusedAssign: enabled: true severity: 1 -ValidBlockPresetSettings: - enabled: true - severity: 0 ValidBlockTarget: enabled: true severity: 0 diff --git a/packages/theme-check-node/configs/recommended.yml b/packages/theme-check-node/configs/recommended.yml index 7dffb3d20..7640452a8 100644 --- a/packages/theme-check-node/configs/recommended.yml +++ b/packages/theme-check-node/configs/recommended.yml @@ -96,9 +96,6 @@ UnknownFilter: UnusedAssign: enabled: true severity: 1 -ValidBlockPresetSettings: - enabled: true - severity: 0 ValidBlockTarget: enabled: true severity: 0 diff --git a/packages/theme-check-node/configs/theme-app-extension.yml b/packages/theme-check-node/configs/theme-app-extension.yml index bb0e66a91..8bd6b46fe 100644 --- a/packages/theme-check-node/configs/theme-app-extension.yml +++ b/packages/theme-check-node/configs/theme-app-extension.yml @@ -19,6 +19,9 @@ AssetSizeAppBlockJavaScript: MissingSchema: enabled: true severity: 0 +NoImportMap: + enabled: true + severity: 0 RequiredLayoutThemeObject: enabled: false severity: 0