-
Notifications
You must be signed in to change notification settings - Fork 32
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
[LiquidDoc] Add snippet hover support inside of {% render %} tag #703
Changes from all commits
931dc9b
fea457f
83897eb
c85a613
52bf118
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 |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
'@shopify/theme-language-server-common': minor | ||
'@shopify/theme-check-common': minor | ||
--- | ||
|
||
Cache liquidDoc fetch results | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
'@shopify/theme-language-server-common': minor | ||
'@shopify/theme-check-common': minor | ||
--- | ||
|
||
Add hover support for Liquid snippets using {% doc %} annotations |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
import { describe, beforeEach, it, expect } from 'vitest'; | ||
import { DocumentManager } from '../../documents'; | ||
import { HoverProvider } from '../HoverProvider'; | ||
import { MetafieldDefinitionMap } from '@shopify/theme-check-common'; | ||
import { GetSnippetDefinitionForURI, SnippetDefinition } from '../../liquidDoc'; | ||
|
||
describe('Module: RenderSnippetHoverProvider', async () => { | ||
let provider: HoverProvider; | ||
let getSnippetDefinition: GetSnippetDefinitionForURI; | ||
const mockSnippetDefinition: SnippetDefinition = { | ||
name: 'product-card', | ||
liquidDoc: { | ||
parameters: [ | ||
{ | ||
name: 'title', | ||
description: 'The title of the product', | ||
type: 'string', | ||
}, | ||
{ | ||
name: 'border-radius', | ||
description: 'The border radius in px', | ||
type: 'number', | ||
}, | ||
], | ||
Comment on lines
+13
to
+24
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. You should add more iterations of parameters (e.g. without description, type, etc) since you have logic behind the templating around them. See review comment above for the ones I've done during tophatting. |
||
}, | ||
}; | ||
|
||
const createProvider = (getSnippetDefinition: GetSnippetDefinitionForURI) => { | ||
return new HoverProvider( | ||
new DocumentManager(), | ||
{ | ||
filters: async () => [], | ||
objects: async () => [], | ||
tags: async () => [], | ||
systemTranslations: async () => ({}), | ||
}, | ||
async (_rootUri: string) => ({} as MetafieldDefinitionMap), | ||
async () => ({}), | ||
async () => [], | ||
getSnippetDefinition, | ||
); | ||
}; | ||
|
||
beforeEach(async () => { | ||
getSnippetDefinition = async () => mockSnippetDefinition; | ||
provider = createProvider(getSnippetDefinition); | ||
}); | ||
|
||
describe('hover', () => { | ||
it('should return snippet definition with all parameters', async () => { | ||
await expect(provider).to.hover( | ||
`{% render 'product-car█d' %}`, | ||
'### product-card\n\n**Parameters:**\n- `title`: string - The title of the product\n- `border-radius`: number - The border radius in px', | ||
); | ||
}); | ||
|
||
it('should return an H3 with snippet name if no LiquidDocDefinition found', async () => { | ||
getSnippetDefinition = async () => ({ name: 'unknown-snippet' }); | ||
provider = createProvider(getSnippetDefinition); | ||
await expect(provider).to.hover(`{% render 'unknown-sni█ppet' %}`, '### unknown-snippet'); | ||
}); | ||
|
||
it('should return nothing if not in render tag', async () => { | ||
await expect(provider).to.hover(`{% assign asdf = 'snip█pet' %}`, null); | ||
await expect(provider).to.hover(`{{ 'snip█pet' }}`, null); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
import { NodeTypes } from '@shopify/liquid-html-parser'; | ||
import { LiquidHtmlNode } from '@shopify/theme-check-common'; | ||
import { Hover, HoverParams } from 'vscode-languageserver'; | ||
import { BaseHoverProvider } from '../BaseHoverProvider'; | ||
import { SnippetDefinition, LiquidDocParameter } from '../../liquidDoc'; | ||
|
||
export class RenderSnippetHoverProvider implements BaseHoverProvider { | ||
constructor( | ||
private getSnippetDefinitionForURI: ( | ||
uri: string, | ||
snippetName: string, | ||
) => Promise<SnippetDefinition>, | ||
) {} | ||
|
||
async hover( | ||
currentNode: LiquidHtmlNode, | ||
ancestors: LiquidHtmlNode[], | ||
params: HoverParams, | ||
): Promise<Hover | null> { | ||
const parentNode = ancestors.at(-1); | ||
if ( | ||
currentNode.type !== NodeTypes.String || | ||
!parentNode || | ||
parentNode.type !== NodeTypes.RenderMarkup | ||
) { | ||
return null; | ||
} | ||
|
||
const snippetName = currentNode.value; | ||
const snippetDefinition = await this.getSnippetDefinitionForURI( | ||
params.textDocument.uri, | ||
jamesmengo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
snippetName, | ||
); | ||
|
||
const liquidDoc = snippetDefinition.liquidDoc; | ||
|
||
if (!liquidDoc) { | ||
return { | ||
contents: { | ||
kind: 'markdown', | ||
value: `### ${snippetDefinition.name}`, | ||
}, | ||
}; | ||
} | ||
|
||
const parts = [`### ${snippetDefinition.name}`]; | ||
|
||
if (liquidDoc.parameters?.length) { | ||
const parameters = liquidDoc.parameters | ||
?.map( | ||
({ name, type, description }: LiquidDocParameter) => | ||
`- \`${name}\`${type ? `: ${type}` : ''} ${description ? `- ${description}` : ''}`, | ||
karreiro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
.join('\n'); | ||
|
||
parts.push('', '**Parameters:**', parameters); | ||
} | ||
|
||
return { | ||
contents: { | ||
kind: 'markdown', | ||
value: parts.join('\n'), | ||
}, | ||
}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
import { expect, it } from 'vitest'; | ||
import { LiquidHtmlNode } from '@shopify/theme-check-common'; | ||
import { toSourceCode } from '@shopify/theme-check-common'; | ||
import { describe } from 'vitest'; | ||
import { getSnippetDefinition } from './liquidDoc'; | ||
|
||
describe('Unit: makeGetLiquidDocDefinitions', () => { | ||
function toAST(code: string) { | ||
return toSourceCode('/tmp/foo.liquid', code).ast as LiquidHtmlNode; | ||
} | ||
|
||
it('should return name if no valid annotations are present in definition', async () => { | ||
const ast = toAST(` | ||
{% doc %} | ||
just a description | ||
@undefined asdf | ||
{% enddoc %} | ||
`); | ||
|
||
const result = getSnippetDefinition(ast, 'product-card'); | ||
expect(result).to.deep.equal({ | ||
name: 'product-card', | ||
liquidDoc: { | ||
parameters: [], | ||
}, | ||
}); | ||
}); | ||
|
||
it('should extract name, description and type from param annotations', async () => { | ||
const ast = toAST(` | ||
{% doc %} | ||
@param {String} firstParam - The first param | ||
@param {Number} secondParam - The second param | ||
@param paramWithNoType - param with no type | ||
@param paramWithOnlyName | ||
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. |
||
{% enddoc %} | ||
`); | ||
|
||
const result = getSnippetDefinition(ast, 'product-card'); | ||
expect(result).to.deep.equal({ | ||
name: 'product-card', | ||
liquidDoc: { | ||
parameters: [ | ||
{ | ||
name: 'firstParam', | ||
description: 'The first param', | ||
type: 'String', | ||
}, | ||
{ | ||
name: 'secondParam', | ||
description: 'The second param', | ||
type: 'Number', | ||
}, | ||
{ | ||
name: 'paramWithNoType', | ||
description: 'param with no type', | ||
type: null, | ||
}, | ||
{ | ||
name: 'paramWithOnlyName', | ||
description: '', | ||
type: null, | ||
}, | ||
], | ||
}, | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,50 @@ | ||||||||||
import { SourceCodeType, visit } from '@shopify/theme-check-common'; | ||||||||||
|
||||||||||
import { LiquidHtmlNode } from '@shopify/theme-check-common'; | ||||||||||
|
||||||||||
import { LiquidDocParamNode } from '@shopify/liquid-html-parser'; | ||||||||||
|
||||||||||
export type GetSnippetDefinitionForURI = ( | ||||||||||
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. I'm placing the types within its own module since this follows the pattern we're using for translations - link This keeps everything self-contained while it's still subject to change as we continue building functionality into theme check, etc. |
||||||||||
uri: string, | ||||||||||
snippetName: string, | ||||||||||
) => Promise<SnippetDefinition>; | ||||||||||
|
||||||||||
export type LiquidDocParameter = { | ||||||||||
name: string; | ||||||||||
description: string | null; | ||||||||||
type: string | null; | ||||||||||
Comment on lines
+14
to
+15
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.
Suggested change
|
||||||||||
}; | ||||||||||
|
||||||||||
export type SnippetDefinition = { | ||||||||||
name: string; | ||||||||||
liquidDoc?: LiquidDocDefinition; | ||||||||||
}; | ||||||||||
|
||||||||||
type LiquidDocDefinition = { | ||||||||||
parameters?: LiquidDocParameter[]; | ||||||||||
}; | ||||||||||
|
||||||||||
export function getSnippetDefinition( | ||||||||||
snippet: LiquidHtmlNode, | ||||||||||
snippetName: string, | ||||||||||
): SnippetDefinition { | ||||||||||
const liquidDocParameters: LiquidDocParameter[] = visit< | ||||||||||
SourceCodeType.LiquidHtml, | ||||||||||
LiquidDocParameter | ||||||||||
>(snippet, { | ||||||||||
LiquidDocParamNode(node: LiquidDocParamNode) { | ||||||||||
return { | ||||||||||
name: node.paramName.value, | ||||||||||
description: node.paramDescription?.value ?? null, | ||||||||||
type: node.paramType?.value ?? null, | ||||||||||
Comment on lines
+38
to
+39
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.
Suggested change
|
||||||||||
}; | ||||||||||
}, | ||||||||||
}); | ||||||||||
|
||||||||||
return { | ||||||||||
name: snippetName, | ||||||||||
liquidDoc: { | ||||||||||
parameters: liquidDocParameters, | ||||||||||
}, | ||||||||||
}; | ||||||||||
} |
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.
this comes from the upstream PR I merged in