Skip to content

Commit 5c0008c

Browse files
aswamyaswamy
authored
Merge pull request #1035 from Shopify/allow-bool-expression-schema-tags
Allow boolean expressions in schema tag
2 parents 6f32cc2 + b94b75d commit 5c0008c

File tree

5 files changed

+127
-96
lines changed

5 files changed

+127
-96
lines changed

‎.changeset/seven-suits-tan.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@shopify/theme-check-common': patch
3+
---
4+
5+
Allow boolean expressions in schema tag

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,17 @@ describe('detectTrailingAssignValue', async () => {
1212
}
1313
});
1414

15+
it('should not report when the boolean expression is in a schema tag', async () => {
16+
const sourceCode = `{% schema %}
17+
{
18+
"visible_if": "{{ this > that }}"
19+
}
20+
{% endschema %}`;
21+
22+
const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode);
23+
expect(offenses).to.have.length(0);
24+
});
25+
1526
it('should report all use of boolean expressions', async () => {
1627
const testCases = [
1728
[`{% assign foo = something == else %}`, '{% assign foo = something %}'],

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

Lines changed: 28 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,33 +1,36 @@
1-
import { NodeTypes } from '@shopify/liquid-html-parser';
2-
import { Check, Context, SourceCodeType } from '../../..';
1+
import { LiquidBooleanExpression, LiquidHtmlNode, NodeTypes } from '@shopify/liquid-html-parser';
2+
import { Problem, SourceCodeType } from '../../..';
33
import { INVALID_SYNTAX_MESSAGE } from './utils';
44

55
export function detectInvalidBooleanExpressions(
6-
context: Context<SourceCodeType.LiquidHtml>,
7-
): Check<SourceCodeType.LiquidHtml> {
8-
return {
9-
async BooleanExpression(node) {
10-
const condition = node.condition;
6+
node: LiquidBooleanExpression,
7+
ancestors: LiquidHtmlNode[],
8+
): Problem<SourceCodeType.LiquidHtml> | undefined {
9+
const condition = node.condition;
10+
11+
// We are okay with boolean expressions in schema tags (specifically for visible_if)
12+
if (
13+
ancestors.some(
14+
(ancestor) => ancestor.type === NodeTypes.LiquidRawTag && ancestor.name === 'schema',
15+
)
16+
) {
17+
return;
18+
}
1119

12-
if (
13-
condition.type !== NodeTypes.Comparison &&
14-
condition.type !== NodeTypes.LogicalExpression
15-
) {
16-
return;
17-
}
20+
if (condition.type !== NodeTypes.Comparison && condition.type !== NodeTypes.LogicalExpression) {
21+
return;
22+
}
1823

19-
context.report({
20-
message: INVALID_SYNTAX_MESSAGE,
21-
startIndex: node.position.start,
22-
endIndex: node.position.end,
23-
fix: (corrector) => {
24-
corrector.replace(
25-
node.position.start,
26-
node.position.end,
27-
node.source.slice(condition.left.position.start, condition.left.position.end),
28-
);
29-
},
30-
});
24+
return {
25+
message: INVALID_SYNTAX_MESSAGE,
26+
startIndex: node.position.start,
27+
endIndex: node.position.end,
28+
fix: (corrector) => {
29+
corrector.replace(
30+
node.position.start,
31+
node.position.end,
32+
node.source.slice(condition.left.position.start, condition.left.position.end),
33+
);
3134
},
3235
};
3336
}

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

Lines changed: 65 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
1-
import { Check, Context, SourceCodeType } from '../../..';
1+
import { LiquidTag } from '@shopify/liquid-html-parser';
2+
import { Problem, SourceCodeType } from '../../..';
23
import { ensureValidAst, INVALID_SYNTAX_MESSAGE } from './utils';
34

45
export function detectMultipleAssignValues(
5-
context: Context<SourceCodeType.LiquidHtml>,
6-
): Check<SourceCodeType.LiquidHtml> {
6+
node: LiquidTag,
7+
): Problem<SourceCodeType.LiquidHtml> | undefined {
78
// Using a regex to match the markup like we do in Shopify/liquid
89
// https://github.com/Shopify/liquid/blob/9bb7fbf123e6e2bd61e00189b1c83159f375d3f3/lib/liquid/tags/assign.rb#L21
910
//
@@ -14,84 +15,79 @@ export function detectMultipleAssignValues(
1415
// 4. The filter section (non-captured)
1516
const ASSIGN_MARKUP_REGEX = /([^=]+)(=\s*)([^|]+)(?:\s*\|\s*.*)?$/m;
1617

17-
return {
18-
async LiquidTag(node) {
19-
if (node.name !== 'assign') {
20-
return;
21-
}
18+
if (node.name !== 'assign') {
19+
return;
20+
}
2221

23-
const markup = node.markup;
22+
const markup = node.markup;
2423

25-
if (typeof markup !== 'string') {
26-
return;
27-
}
24+
if (typeof markup !== 'string') {
25+
return;
26+
}
2827

29-
const match = markup.match(ASSIGN_MARKUP_REGEX);
30-
if (!match) {
31-
return;
32-
}
28+
const match = markup.match(ASSIGN_MARKUP_REGEX);
29+
if (!match) {
30+
return;
31+
}
3332

34-
// If we have a markup 'foo = "123" something | upcase: 123', we have the following groups
35-
const [
36-
// 'foo = "123" something | upcase: 123'
37-
_fullMatch,
38-
// 'foo '
39-
assignmentVariable,
40-
// '= '
41-
assignmentOperator,
42-
// '"123" something'
43-
assignmentValue,
44-
] = match;
33+
// If we have a markup 'foo = "123" something | upcase: 123', we have the following groups
34+
const [
35+
// 'foo = "123" something | upcase: 123'
36+
_fullMatch,
37+
// 'foo '
38+
assignmentVariable,
39+
// '= '
40+
assignmentOperator,
41+
// '"123" something'
42+
assignmentValue,
43+
] = match;
4544

46-
// Only capture the first item in the value section
47-
const firstValueMatch = assignmentValue.match(/"[^"]*"|'[^']*'|\S+/);
48-
const firstAssignmentValue = firstValueMatch ? firstValueMatch[0] : null;
45+
// Only capture the first item in the value section
46+
const firstValueMatch = assignmentValue.match(/"[^"]*"|'[^']*'|\S+/);
47+
const firstAssignmentValue = firstValueMatch ? firstValueMatch[0] : null;
4948

50-
if (!firstAssignmentValue) {
51-
return;
52-
}
49+
if (!firstAssignmentValue) {
50+
return;
51+
}
5352

54-
const removalIndices = (source: string) => {
55-
const offset = source.indexOf(markup);
53+
const removalIndices = (source: string) => {
54+
const offset = source.indexOf(markup);
5655

57-
return {
58-
startIndex:
59-
offset +
60-
assignmentVariable.length +
61-
assignmentOperator.length +
62-
firstAssignmentValue.length,
63-
endIndex:
64-
offset +
65-
assignmentVariable.length +
66-
assignmentOperator.length +
67-
assignmentValue.trimEnd().length,
68-
};
69-
};
56+
return {
57+
startIndex:
58+
offset +
59+
assignmentVariable.length +
60+
assignmentOperator.length +
61+
firstAssignmentValue.length,
62+
endIndex:
63+
offset +
64+
assignmentVariable.length +
65+
assignmentOperator.length +
66+
assignmentValue.trimEnd().length,
67+
};
68+
};
7069

71-
const tagSource = node.source.slice(node.position.start, node.position.end);
72-
const { startIndex: tagSourceRemovalStartIndex, endIndex: tagSourceRemovalEndIndex } =
73-
removalIndices(tagSource);
70+
const tagSource = node.source.slice(node.position.start, node.position.end);
71+
const { startIndex: tagSourceRemovalStartIndex, endIndex: tagSourceRemovalEndIndex } =
72+
removalIndices(tagSource);
7473

75-
if (
76-
!ensureValidAst(
77-
tagSource.slice(0, tagSourceRemovalStartIndex) +
78-
tagSource.slice(tagSourceRemovalEndIndex),
79-
)
80-
) {
81-
// If the new AST is invalid, we don't want to auto-fix it
82-
return;
83-
}
74+
if (
75+
!ensureValidAst(
76+
tagSource.slice(0, tagSourceRemovalStartIndex) + tagSource.slice(tagSourceRemovalEndIndex),
77+
)
78+
) {
79+
// If the new AST is invalid, we don't want to auto-fix it
80+
return;
81+
}
8482

85-
const { startIndex, endIndex } = removalIndices(node.source);
83+
const { startIndex, endIndex } = removalIndices(node.source);
8684

87-
context.report({
88-
message: INVALID_SYNTAX_MESSAGE,
89-
startIndex,
90-
endIndex,
91-
fix: (corrector) => {
92-
corrector.replace(startIndex, endIndex, '');
93-
},
94-
});
85+
return {
86+
message: INVALID_SYNTAX_MESSAGE,
87+
startIndex,
88+
endIndex,
89+
fix: (corrector) => {
90+
corrector.replace(startIndex, endIndex, '');
9591
},
9692
};
9793
}

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

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,24 @@ export const LiquidHTMLSyntaxError: LiquidCheckDefinition = {
3939
const ast = context.file.ast;
4040
if (!isError(ast)) {
4141
return {
42-
...detectInvalidBooleanExpressions(context),
43-
...detectMultipleAssignValues(context),
42+
async BooleanExpression(node, ancestors) {
43+
const problem = detectInvalidBooleanExpressions(node, ancestors);
44+
45+
if (!problem) {
46+
return;
47+
}
48+
49+
context.report(problem);
50+
},
51+
async LiquidTag(node) {
52+
const problem = detectMultipleAssignValues(node);
53+
54+
if (!problem) {
55+
return;
56+
}
57+
58+
context.report(problem);
59+
},
4460
};
4561
}
4662

0 commit comments

Comments
 (0)