Merge pull request #1038 from Shopify/find-and-fix-no-echo-value

Find+fix echo tags & variable output with no value

Author: aswamy

.changeset/hip-scissors-accept.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme-check-common': patch
+---
+
+Find+fix echo tags & variable output with no value

…-error/checks/MultipleEchoValues.spec.ts …ax-error/checks/InvalidEchoValue.spec.tspackages/theme-check-common/src/checks/liquid-html-syntax-error/checks/MultipleEchoValues.spec.ts renamed to packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidEchoValue.spec.ts packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/MultipleEchoValues.spec.ts renamed to packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidEchoValue.spec.ts

@@ -16,13 +16,16 @@ vi.mock('@shopify/liquid-html-parser', async (importOriginal) => {
   };
 });
 
-describe('detectMultipleEchoValues', async () => {
-  it('should not report when there are no trailing values', async () => {
+describe('detectInvalidEchoValue', async () => {
+  it('should not report when echo value is valid', async () => {
     const testCases = [
       `{% echo '123' %}`,
       `{% echo '123' | upcase %}`,
       `{{ '123' }}`,
       `{{ '123' | upcase }}`,
+      `{{ }}`,
+      `{{ echo }}`,
+      `{% liquid echo %}`,
     ];
 
     for (const sourceCode of testCases) {
@@ -75,6 +78,23 @@ describe('detectMultipleEchoValues', async () => {
     }
   });
 
+  it('should report when there is no value', async () => {
+    const testCases = [
+      [`{% echo | upcase %}`, '{% echo blank %}'],
+      [`{{ | upcase }}`, `{{ blank }}`],
+      [`{% liquid echo | upcase %}`, `{% liquid echo blank %}`],
+    ];
+
+    for (const [sourceCode, expected] of testCases) {
+      const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
+      expect(offenses).to.have.length(1);
+      expect(offenses[0].message).to.equal('Syntax is not supported');
+
+      const fixed = applyFix(sourceCode, offenses[0]);
+      expect(fixed).to.equal(expected);
+    }
+  });
+
   it('should not report when the fixed code produces an invalid AST', async () => {
     const sourceCode = `{% echo '123' 555 text %}`;
 

…yntax-error/checks/MultipleEchoValues.ts …-syntax-error/checks/InvalidEchoValue.tspackages/theme-check-common/src/checks/liquid-html-syntax-error/checks/MultipleEchoValues.ts renamed to packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidEchoValue.ts packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/MultipleEchoValues.ts renamed to packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/InvalidEchoValue.ts

@@ -1,8 +1,8 @@
 import { LiquidTag, LiquidVariableOutput, NodeTypes } from '@shopify/liquid-html-parser';
 import { Problem, SourceCodeType } from '../../..';
-import { ensureValidAst, getFirstValueInMarkup, INVALID_SYNTAX_MESSAGE } from './utils';
+import { ensureValidAst, getValuesInMarkup, INVALID_SYNTAX_MESSAGE } from './utils';
 
-export function detectMultipleEchoValues(
+export function detectInvalidEchoValue(
   node: LiquidTag | LiquidVariableOutput,
 ): Problem<SourceCodeType.LiquidHtml> | undefined {
   // We've broken it up into two groups:
@@ -16,7 +16,12 @@ export function detectMultipleEchoValues(
 
   const markup = node.markup;
 
-  if (typeof markup !== 'string') {
+  if (
+    typeof markup !== 'string' ||
+    // echo tags and variable outputs without markup are strict-valid:
+    // e.g. {{ }}, {% echo %}, and {% liquid echo %}
+    !markup
+  ) {
     return;
   }
 
@@ -27,10 +32,20 @@ export function detectMultipleEchoValues(
 
   const [, echoValue] = match;
 
-  const firstEchoValue = getFirstValueInMarkup(echoValue);
+  const firstEchoValue = getValuesInMarkup(echoValue).at(0)?.value;
 
   if (!firstEchoValue) {
-    return;
+    const startIndex = node.source.indexOf(markup, node.position.start);
+    const endIndex = startIndex + markup.length;
+
+    return {
+      message: INVALID_SYNTAX_MESSAGE,
+      startIndex,
+      endIndex,
+      fix: (corrector) => {
+        corrector.replace(startIndex, endIndex, 'blank');
+      },
+    };
   }
 
   const removalIndices = (source: string) => {

packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/MultipleAssignValues.ts

@@ -1,6 +1,6 @@
 import { LiquidTag } from '@shopify/liquid-html-parser';
 import { Problem, SourceCodeType } from '../../..';
-import { ensureValidAst, getFirstValueInMarkup, INVALID_SYNTAX_MESSAGE } from './utils';
+import { ensureValidAst, getValuesInMarkup, INVALID_SYNTAX_MESSAGE } from './utils';
 
 export function detectMultipleAssignValues(
   node: LiquidTag,
@@ -42,7 +42,7 @@ export function detectMultipleAssignValues(
     assignmentValue,
   ] = match;
 
-  const firstAssignmentValue = getFirstValueInMarkup(assignmentValue);
+  const firstAssignmentValue = getValuesInMarkup(assignmentValue).at(0)?.value;
 
   if (!firstAssignmentValue) {
     return;

packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/utils.spec.ts

@@ -0,0 +1,30 @@
+import { expect, describe, it } from 'vitest';
+import { getValuesInMarkup } from './utils';
+
+describe('getValuesInMarkup', () => {
+  it('should return the values in the markup (one value)', () => {
+    const markup = '"123"';
+    const values = getValuesInMarkup(markup);
+    expect(values).toContainEqual({ value: '"123"', index: 0 });
+  });
+
+  it('should return the values in the markup (multiple values)', () => {
+    const markup = '"123" 555 text';
+    const values = getValuesInMarkup(markup);
+    expect(values).toContainEqual({ value: '"123"', index: 0 });
+    expect(values).toContainEqual({ value: '555', index: 6 });
+    expect(values).toContainEqual({ value: 'text', index: 10 });
+  });
+
+  it('should return nothing when no values in the markup', () => {
+    const markup = '';
+    const values = getValuesInMarkup(markup);
+    expect(values).to.have.length(0);
+  });
+
+  it('should return nothing when only spaces in the markup', () => {
+    const markup = '     ';
+    const values = getValuesInMarkup(markup);
+    expect(values).to.have.length(0);
+  });
+});

packages/theme-check-common/src/checks/liquid-html-syntax-error/checks/utils.ts

@@ -12,7 +12,9 @@ export function ensureValidAst(source: string) {
   }
 }
 
-export function getFirstValueInMarkup(markup: string) {
-  const match = markup.match(/"[^"]*"|'[^']*'|\S+/);
-  return match?.at(0);
+export function getValuesInMarkup(markup: string) {
+  return [...markup.matchAll(/"[^"]*"|'[^']*'|\S+/g)].map((match) => ({
+    value: match[0],
+    index: match.index,
+  }));
 }

packages/theme-check-common/src/checks/liquid-html-syntax-error/index.ts

@@ -2,7 +2,7 @@ import { Severity, SourceCodeType, LiquidCheckDefinition } from '../../types';
 import { getOffset, isError } from '../../utils';
 import { detectMultipleAssignValues } from './checks/MultipleAssignValues';
 import { detectInvalidBooleanExpressions } from './checks/InvalidBooleanExpressions';
-import { detectMultipleEchoValues as detectMultipleEchoValues } from './checks/MultipleEchoValues';
+import { detectInvalidEchoValue } from './checks/InvalidEchoValue';
 
 type LineColPosition = {
   line: number;
@@ -50,7 +50,7 @@ export const LiquidHTMLSyntaxError: LiquidCheckDefinition = {
           context.report(problem);
         },
         async LiquidTag(node) {
-          const problem = detectMultipleAssignValues(node) || detectMultipleEchoValues(node);
+          const problem = detectMultipleAssignValues(node) || detectInvalidEchoValue(node);
 
           if (!problem) {
             return;
@@ -59,7 +59,7 @@ export const LiquidHTMLSyntaxError: LiquidCheckDefinition = {
           context.report(problem);
         },
         async LiquidVariableOutput(node) {
-          const problem = detectMultipleEchoValues(node);
+          const problem = detectInvalidEchoValue(node);
 
           if (!problem) {
             return;