[LiquidDoc] Add snippet hover support inside of {% render %} tag #703

jamesmengo · 2025-01-13T19:00:15Z

What are you adding in this PR?

https://github.com/Shopify/develop-advanced-edits/issues/496

1) Hover support for Liquid snippets inside of a {% render %} tag. Users will now see:

The snippet name as a header
A list of parameters with their types and descriptions (if
documented)

If there is no {% doc %} present in the snippet, we will just render the snippet name

2) Caching for fetching LiquidDoc definitions

Example snippet documentation:

{% doc %}
  @param {String} title - The title of the product
  @param {Number} border-radius - The border radius in px
{% enddoc %}

When hovering over `product-card` in `{% render 'product-card' %}`, users will see:

### product-card

**Parameters:**
- `title`: String - The title of the product
- `border-radius`: Number - The border radius in px

Uploading Cursor - liquidDoc.ts — theme-tools.mp4…

Hovering a snippet with docs

Hovering a snippet without liquiddoc

What's next? Any followup issues?

Consider adding validation for param types
Add support for return value documentation
Add support for example usage documentation

What did you learn?

This is my first time working with the hover API in VS code. Pretty cool to read up on!

Before you deploy

I included a minor bump changeset
My feature is backward compatible

jamesmengo · 2025-01-13T19:00:54Z

[LiquidDoc] Add snippet hover support inside of {% render %} tag #703 : 3 dependent PRs (#719 , #726 , #729 ) 👈 (View in Graphite)
Add parsing + prettier support for @param in {% doc %} tags #646 : 3 other dependent PRs (#652 , #665 , #691 )
Add basic {% doc %} tag parsing support #621 : 2 other dependent PRs (#636 , #638 )
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

packages/theme-language-server-common/src/server/startServer.ts

packages/theme-check-common/src/context-utils.ts

jamesmengo · 2025-01-22T00:58:31Z

.changeset/dry-donkeys-behave.md

+'@shopify/theme-check-common': minor
+---
+
+Cache liquidDoc fetch results


this comes from the upstream PR I merged in

jamesmengo · 2025-01-22T01:04:03Z

packages/theme-language-server-common/src/liquidDoc.ts

+
+import { LiquidDocParamNode } from '@shopify/liquid-html-parser';
+
+export type GetSnippetDefinitionForURI = (


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.

packages/theme-language-server-common/src/documents/DocumentManager.ts

packages/theme-language-server-common/src/hover/HoverProvider.ts

packages/theme-language-server-common/src/hover/providers/RenderSnippetHoverProvider.ts

packages/theme-language-server-common/src/server/startServer.ts

aswamy

Did basic tophatting seems good

Minor comments

packages/theme-language-server-common/src/hover/providers/RenderSnippetHoverProvider.ts

karreiro

Thank you for this PR, @jamesmengo! Great stuff 🎩

I've left only two minor comments that I'd love to know your thoughts :)

packages/theme-language-server-common/src/documents/DocumentManager.ts

packages/theme-language-server-common/src/hover/providers/RenderSnippetHoverProvider.ts

aswamy

Looking good James 😎 Small comments, but none blocking merge

I put a breakpoint in the memoized getLiquidDoc and looks like it's working well.

aswamy · 2025-01-24T20:44:00Z

packages/theme-language-server-common/src/liquidDoc.ts

+  description: string | null;
+  type: string | null;


Suggested change

description: string | null;

type: string | null;

description?: string;

type?: string;

aswamy · 2025-01-24T20:44:13Z

packages/theme-language-server-common/src/liquidDoc.ts

+        description: node.paramDescription?.value ?? null,
+        type: node.paramType?.value ?? null,


Suggested change

description: node.paramDescription?.value ?? null,

type: node.paramType?.value ?? null,

description: node.paramDescription?.value,

type: node.paramType?.value,

aswamy · 2025-01-24T20:47:52Z

packages/theme-language-server-common/src/liquidDoc.spec.ts

+          @param {String} firstParam - The first param
+          @param {Number} secondParam - The second param
+          @param paramWithNoType - param with no type
+          @param paramWithOnlyName


You should also have:

1 param with type but no description

1 @unsupport-param

1 free-form text

aswamy · 2025-01-24T20:53:43Z

packages/theme-language-server-common/src/hover/providers/RenderSnippetHoverProvider.spec.ts

+      parameters: [
+        {
+          name: 'title',
+          description: 'The title of the product',
+          type: 'string',
+        },
+        {
+          name: 'border-radius',
+          description: 'The border radius in px',
+          type: 'number',
+        },
+      ],


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.

Add util function to read liquid doc definitions for a snippet Implement SnippetHoverProvider Connect SnippetHoverProvider with server Add changeset Provide default value for getLiquidDocDefinitionsForURI Rename SnippetHoverProvider -> RenderSnippetHoverProvider

…wn module ---- We didn't need to use the make pattern at least at this stage, so I'm removing the code in ThemeCheckCommon and placing it in its own module. We may need to revisit this when we are writing theme checks. However, for the purpose of hover, this is where the code should live.

…clarity - Renamed `GetLiquidDocDefinitionsForURI` to `GetSnippetDefinitionForURI` - Introduced `SnippetDefinition` type to replace `LiquidDocDefinition`, enhancing type specificity.

karreiro

Thank you, @jamesmengo! LGTM and works as expected as well 🎩 :)

jamesmengo · 2025-01-27T17:13:50Z

Merge activity

Jan 27, 12:13 PM EST: A user started a stack merge that includes this pull request via Graphite.
Jan 27, 12:14 PM EST: A user merged this pull request with Graphite.

jamesmengo mentioned this pull request Jan 13, 2025

Add punctuation support in LiquidDoc param descriptions #691

Merged

8 tasks

jamesmengo force-pushed the jm/param_hover branch from 11023d6 to c7b2d3b Compare January 13, 2025 20:51

jamesmengo force-pushed the jm/implement_fallbackNode branch from 9251a0f to d10e268 Compare January 13, 2025 21:01

jamesmengo force-pushed the jm/param_hover branch from c7b2d3b to 38316c6 Compare January 13, 2025 21:01

jamesmengo force-pushed the jm/implement_fallbackNode branch from d10e268 to 4e69c7c Compare January 13, 2025 21:10

jamesmengo force-pushed the jm/param_hover branch from 38316c6 to a58b45c Compare January 13, 2025 21:10

jamesmengo force-pushed the jm/implement_fallbackNode branch from c132c46 to 915e388 Compare January 14, 2025 02:53

jamesmengo force-pushed the jm/param_hover branch 6 times, most recently from 6ce56e3 to 3b20751 Compare January 15, 2025 00:54

jamesmengo requested review from aswamy and charlespwd January 15, 2025 00:55

jamesmengo commented Jan 15, 2025

View reviewed changes

packages/theme-language-server-common/src/server/startServer.ts Outdated Show resolved Hide resolved

charlespwd reviewed Jan 15, 2025

View reviewed changes

packages/theme-check-common/src/context-utils.ts Outdated Show resolved Hide resolved

jamesmengo changed the base branch from jm/implement_fallbackNode to graphite-base/703 January 16, 2025 22:17

jamesmengo force-pushed the jm/param_hover branch from 3b20751 to e1c6500 Compare January 16, 2025 22:20

jamesmengo force-pushed the graphite-base/703 branch from 3c9cefa to 6ab6856 Compare January 16, 2025 22:20

jamesmengo changed the base branch from graphite-base/703 to main January 16, 2025 22:20

jamesmengo force-pushed the jm/param_hover branch 3 times, most recently from b6ca8f0 to 35d1735 Compare January 17, 2025 01:29

jamesmengo requested a review from karreiro January 22, 2025 00:24

jamesmengo changed the title ~~[LiquidDoc] Render @Param tag on hover~~ [LiquidDoc] Add snippet hover support inside of {% render %} tag Jan 22, 2025

jamesmengo mentioned this pull request Jan 22, 2025

Cache liquidDoc definitions in DocumentManager #726

Closed

8 tasks

jamesmengo force-pushed the jm/param_hover branch 2 times, most recently from 3afb318 to a167761 Compare January 22, 2025 00:46

jamesmengo commented Jan 22, 2025

View reviewed changes

jamesmengo self-assigned this Jan 22, 2025

charlespwd reviewed Jan 22, 2025

View reviewed changes

aswamy reviewed Jan 22, 2025

View reviewed changes

jamesmengo force-pushed the jm/param_hover branch from ef399e2 to 681ba72 Compare January 23, 2025 01:53

jamesmengo mentioned this pull request Jan 23, 2025

[LiquidDoc] Prevent rendering of hover tooltip for snippets with no renderable content #729

Merged

8 tasks

karreiro reviewed Jan 23, 2025

View reviewed changes

packages/theme-language-server-common/src/documents/DocumentManager.ts Show resolved Hide resolved

packages/theme-language-server-common/src/documents/DocumentManager.ts Show resolved Hide resolved

jamesmengo commented Jan 23, 2025

View reviewed changes

packages/theme-language-server-common/src/hover/providers/RenderSnippetHoverProvider.ts Show resolved Hide resolved

jamesmengo requested review from karreiro, aswamy and charlespwd January 23, 2025 19:22

aswamy approved these changes Jan 24, 2025

View reviewed changes

jamesmengo added 5 commits January 24, 2025 15:35

Add types

931dc9b

Add util function to read liquid doc definitions for a snippet Implement SnippetHoverProvider Connect SnippetHoverProvider with server Add changeset Provide default value for getLiquidDocDefinitionsForURI Rename SnippetHoverProvider -> RenderSnippetHoverProvider

Refactor Liquid documentation types and update references to improve …

83897eb

…clarity - Renamed `GetLiquidDocDefinitionsForURI` to `GetSnippetDefinitionForURI` - Introduced `SnippetDefinition` type to replace `LiquidDocDefinition`, enhancing type specificity.

Cache liquidDoc definitions in DocumentManager

c85a613

Rename liquidDoc to getLiquidDoc

52bf118

jamesmengo force-pushed the jm/param_hover branch from 681ba72 to 52bf118 Compare January 25, 2025 00:17

This was referenced Jan 25, 2025

[LiquidDoc] Parser support for optional parameters #733

Draft

[LiquidDoc] Formatting support for optional @param delimiters [] #734

Draft

[LiquidDoc] Hover: Wrap optional parameter names in [] #735

Draft

karreiro approved these changes Jan 27, 2025

View reviewed changes

jamesmengo merged commit a42459e into main Jan 27, 2025
7 checks passed

jamesmengo deleted the jm/param_hover branch January 27, 2025 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LiquidDoc] Add snippet hover support inside of {% render %} tag #703

[LiquidDoc] Add snippet hover support inside of {% render %} tag #703

jamesmengo commented Jan 13, 2025 •

edited

Loading

jamesmengo commented Jan 13, 2025 •

edited

Loading

jamesmengo Jan 22, 2025

jamesmengo Jan 22, 2025 •

edited

Loading

aswamy left a comment

karreiro left a comment

aswamy left a comment

aswamy Jan 24, 2025

aswamy Jan 24, 2025

aswamy Jan 24, 2025

aswamy Jan 24, 2025

karreiro left a comment

jamesmengo commented Jan 27, 2025 •

edited

Loading


		import { LiquidDocParamNode } from '@shopify/liquid-html-parser';

		export type GetSnippetDefinitionForURI = (

		description: node.paramDescription?.value ?? null,
		type: node.paramType?.value ?? null,

[LiquidDoc] Add snippet hover support inside of {% render %} tag #703

[LiquidDoc] Add snippet hover support inside of {% render %} tag #703

Conversation

jamesmengo commented Jan 13, 2025 • edited Loading

What are you adding in this PR?

Example snippet documentation:

When hovering over product-card in {% render 'product-card' %}, users will see:

Hovering a snippet with docs

Hovering a snippet without liquiddoc

What's next? Any followup issues?

What did you learn?

Before you deploy

jamesmengo commented Jan 13, 2025 • edited Loading

jamesmengo Jan 22, 2025

Choose a reason for hiding this comment

jamesmengo Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

aswamy left a comment

Choose a reason for hiding this comment

karreiro left a comment

Choose a reason for hiding this comment

aswamy left a comment

Choose a reason for hiding this comment

aswamy Jan 24, 2025

Choose a reason for hiding this comment

aswamy Jan 24, 2025

Choose a reason for hiding this comment

aswamy Jan 24, 2025

Choose a reason for hiding this comment

aswamy Jan 24, 2025

Choose a reason for hiding this comment

karreiro left a comment

Choose a reason for hiding this comment

jamesmengo commented Jan 27, 2025 • edited Loading

Merge activity

jamesmengo commented Jan 13, 2025 •

edited

Loading

When hovering over `product-card` in `{% render 'product-card' %}`, users will see:

jamesmengo commented Jan 13, 2025 •

edited

Loading

jamesmengo Jan 22, 2025 •

edited

Loading

jamesmengo commented Jan 27, 2025 •

edited

Loading