-
Notifications
You must be signed in to change notification settings - Fork 29
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 () => { | ||||||
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 GitHub Actions / Tests / OS ubuntu-latest / NodeJS 18packages/theme-language-server-common/src/documentLinks/DocumentLinksProvider.spec.ts > DocumentLinksProvider > should return a list of document links with correct URLs for a LiquidRawTag document
Check failure on line 78 in packages/theme-language-server-common/src/documentLinks/DocumentLinksProvider.spec.ts GitHub Actions / Tests / OS ubuntu-latest / NodeJS 20packages/theme-language-server-common/src/documentLinks/DocumentLinksProvider.spec.ts > DocumentLinksProvider > should return a list of document links with correct URLs for a LiquidRawTag document
|
||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Copilot is powered by AI, so mistakes are possible. Review output carefully before use. |
||||||
}); | ||||||
}); |
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'; | ||
|
@@ -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, | ||
|
@@ -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 []; | ||
}, | ||
}; | ||
} | ||
|
||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} |
There was a problem hiding this comment.
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.