Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -61,4 +61,20 @@ describe('DocumentLinksProvider', () => {
expect(result[i].target).toBe(expectedUrls[i]);
}
});

it('should return a list of document links with correct URLs for a LiquidRawTag document', async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of this unit test and what its verifying for doesnt quite match. I do think that the test case you are describing here should be tested in addition to the empty array case you implemented for.

uriString = 'file:///path/to/liquid-raw-tag-document.liquid';
rootUri = 'file:///path/to/project';

const liquidRawTagContent = `
{% schema %}
{ "blocks": [{ "type": "valid" }] }
{% endschema %}
`;

documentManager.open(uriString, liquidRawTagContent, 1);

const result = await documentLinksProvider.documentLinks(uriString);
expect(result).toEqual([]);
Copy link

Copilot AI Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test case description suggests it should return a list of document links with correct URLs, but the expectation is an empty array. This should be corrected to check for the correct URLs.

Suggested change
expect(result).toEqual([]);
expect(result).toEqual(expectedUrls);

Copilot uses AI. Check for mistakes.
});
});
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { LiquidHtmlNode, LiquidString, NodeTypes } from '@shopify/liquid-html-parser';
import { LiquidHtmlNode, LiquidRawTag, LiquidString, NodeTypes } from '@shopify/liquid-html-parser';
import { SourceCodeType } from '@shopify/theme-check-common';
import { DocumentLink, Range } from 'vscode-languageserver';
import { TextDocument } from 'vscode-languageserver-textdocument';
Expand All @@ -7,6 +7,8 @@ import { URI, Utils } from 'vscode-uri';
import { DocumentManager } from '../documents';
import { visit, Visitor } from '@shopify/theme-check-common';

import { parseTree, findNodeAtLocation, ParseError, Node as JSONNode } from 'jsonc-parser';

export class DocumentLinksProvider {
constructor(
private documentManager: DocumentManager,
Expand Down Expand Up @@ -79,6 +81,42 @@ function documentLinksVisitor(
Utils.resolvePath(root, 'assets', expression.value).toString(),
);
},

LiquidRawTag(node) {
// look for schema tags
if (node.name === 'schema') {
// parse and return a tree of the schema
const errors: ParseError[] = [];
const jsonNode = parseTree(node.body.value, errors);
if (!jsonNode || errors.length > 0) {
return [];
}

// create an array of links so we can process all block types and preset block types in the schema
const links: DocumentLink[] = [];

// Process top-level blocks
const blocksNode = findNodeAtLocation(jsonNode, ['blocks']);
if (blocksNode && blocksNode.type === 'array' && blocksNode.children) {
links.push(...createLinksFromBlocks(blocksNode, node, textDocument, root));
}

// Process presets
const presetsNode = findNodeAtLocation(jsonNode, ['presets']);
if (presetsNode && presetsNode.type === 'array' && presetsNode.children) {
presetsNode.children.forEach((presetNode) => {
// Process blocks within each preset
const presetBlocksNode = findNodeAtLocation(presetNode, ['blocks']);
if (presetBlocksNode) {
links.push(...processPresetBlocks(presetBlocksNode, node, textDocument, root));
}
});
}

return links;
}
return [];
},
};
}

Expand All @@ -91,3 +129,149 @@ function range(textDocument: TextDocument, node: { position: LiquidHtmlNode['pos
function isLiquidString(node: LiquidHtmlNode): node is LiquidString {
return node.type === NodeTypes.String;
}

function createDocumentLinkForTypeNode(
typeNode: JSONNode,
parentNode: LiquidRawTag,
textDocument: TextDocument,
root: URI,
blockType: string,
): DocumentLink | null {
const startOffset = typeNode.offset;
const endOffset = typeNode.offset + typeNode.length;
const startPos = parentNode.body.position.start + startOffset;
const endPos = parentNode.body.position.start + endOffset;

const start = textDocument.positionAt(startPos);
const end = textDocument.positionAt(endPos);

return DocumentLink.create(
Range.create(start, end),
Utils.resolvePath(root, 'blocks', `${blockType}.liquid`).toString(),
);
}

function processPresetBlocks(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of a hefty function. it would help with scanability to encapsulate common functionality between the conditional branches.

blocksNode: JSONNode,
parentNode: LiquidRawTag,
textDocument: TextDocument,
root: URI,
): DocumentLink[] {
const links: DocumentLink[] = [];

if (blocksNode.type === 'object' && blocksNode.children) {
blocksNode.children.forEach((propertyNode) => {
const blockValueNode = propertyNode.children?.[1]; // The value node of the property
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its always the second argument of the children array?

if (!blockValueNode) return;

// Check if the block has a 'name' key so we don't deeplink inline block types
const nameNode = findNodeAtLocation(blockValueNode, ['name']);
if (nameNode) {
return;
}

const typeNode = findNodeAtLocation(blockValueNode, ['type']);
if (typeNode && typeNode.type === 'string' && typeof typeNode.value === 'string') {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When evaluating a generic variable conforms to a specific structure, you can do something called "type narrowing" when there is a specific type definition for the structure of data that you're verifying for. This has typescript benefits of showing proper typing feedback as you write code inside these conditionals. Take a look at these examples of type narrowing code in the theme-tools repo: https://github.com/Shopify/theme-tools/blob/deeplink-theme-blocks/packages/theme-check-common/src/types.ts#L23-L25

const blockType = typeNode.value;
if (blockType.startsWith('@')) {
return;
}

const link = createDocumentLinkForTypeNode(
typeNode,
parentNode,
textDocument,
root,
blockType,
);

if (link) {
links.push(link);
}
}

// Recursively process nested blocks
const nestedBlocksNode = findNodeAtLocation(blockValueNode, ['blocks']);
if (nestedBlocksNode) {
links.push(...processPresetBlocks(nestedBlocksNode, parentNode, textDocument, root));
}
});
} else if (blocksNode.type === 'array' && blocksNode.children) {
blocksNode.children.forEach((blockNode) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way to reduce this big function is to move the iteree code within the forEach into a standalone function definition.

// Check if the block has a 'name' key
const nameNode = findNodeAtLocation(blockNode, ['name']);
if (nameNode) {
return; // Skip creating a link if 'name' key exists
}

const typeNode = findNodeAtLocation(blockNode, ['type']);
if (typeNode && typeNode.type === 'string' && typeof typeNode.value === 'string') {
const blockType = typeNode.value;
if (blockType.startsWith('@')) {
return;
}

const link = createDocumentLinkForTypeNode(
typeNode,
parentNode,
textDocument,
root,
blockType,
);

if (link) {
links.push(link);
}
}

// Recursively process nested blocks
const nestedBlocksNode = findNodeAtLocation(blockNode, ['blocks']);
if (nestedBlocksNode) {
links.push(...processPresetBlocks(nestedBlocksNode, parentNode, textDocument, root));
}
});
}

return links;
}

function createLinksFromBlocks(
blocksNode: JSONNode,
parentNode: LiquidRawTag,
textDocument: TextDocument,
root: URI,
): DocumentLink[] {
const links: DocumentLink[] = [];

if (blocksNode.children) {
blocksNode.children.forEach((blockNode: JSONNode) => {
// Check if the block has a 'name' key to avoid deeplinking inline block types
const nameNode = findNodeAtLocation(blockNode, ['name']);
if (nameNode) {
return;
}

const typeNode = findNodeAtLocation(blockNode, ['type']);
if (typeNode && typeNode.type === 'string' && typeof typeNode.value === 'string') {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment about type narrowing this to something more specific from JSONNode

const blockType = typeNode.value;
if (blockType.startsWith('@')) {
return;
}

const link = createDocumentLinkForTypeNode(
typeNode,
parentNode,
textDocument,
root,
blockType,
);

if (link) {
links.push(link);
}
}
});
}

return links;
}