Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add deeplinking for block types #674

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
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 @@
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
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([]);

Check failure on line 78 in packages/theme-language-server-common/src/documentLinks/DocumentLinksProvider.spec.ts

View workflow job for this annotation

GitHub Actions / Tests / OS ubuntu-latest / NodeJS 18

packages/theme-language-server-common/src/documentLinks/DocumentLinksProvider.spec.ts > DocumentLinksProvider > should return a list of document links with correct URLs for a LiquidRawTag document

AssertionError: expected [ { range: { …(2) }, …(2) } ] to deeply equal [] - Expected + Received - Array [] + Array [ + Object { + "data": undefined, + "range": Object { + "end": Object { + "character": 38, + "line": 2, + }, + "start": Object { + "character": 31, + "line": 2, + }, + }, + "target": "file:///path/to/project/blocks/valid.liquid", + }, + ] ❯ packages/theme-language-server-common/src/documentLinks/DocumentLinksProvider.spec.ts:78:20

Check failure on line 78 in packages/theme-language-server-common/src/documentLinks/DocumentLinksProvider.spec.ts

View workflow job for this annotation

GitHub Actions / Tests / OS ubuntu-latest / NodeJS 20

packages/theme-language-server-common/src/documentLinks/DocumentLinksProvider.spec.ts > DocumentLinksProvider > should return a list of document links with correct URLs for a LiquidRawTag document

AssertionError: expected [ { range: { …(2) }, …(2) } ] to deeply equal [] - Expected + Received - Array [] + Array [ + Object { + "data": undefined, + "range": Object { + "end": Object { + "character": 38, + "line": 2, + }, + "start": Object { + "character": 31, + "line": 2, + }, + }, + "target": "file:///path/to/project/blocks/valid.liquid", + }, + ] ❯ packages/theme-language-server-common/src/documentLinks/DocumentLinksProvider.spec.ts:78:20
Copy link
Preview

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 is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
});
});
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(
blocksNode: JSONNode,
Copy link
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.

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
if (!blockValueNode) return;
Copy link
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?


// 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') {
const blockType = typeNode.value;
Copy link
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

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) => {
// Check if the block has a 'name' key
Copy link
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.

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') {
const blockType = typeNode.value;
Copy link
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

if (blockType.startsWith('@')) {
return;
}

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

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

return links;
}
Loading