Improve the JSON completion & hover building blocks #622

charlespwd · 2024-11-28T19:07:19Z

What are you adding in this PR?

The json-languageservice API was a bit awkward to use. Had a bunch of weird type problems that nobody should really have to care about.

In this PR, I use an adapter to make folks write Completion & Hover providers in a manner similar to what we do with liquid files.

It's not 100% the same, but it's similar enough and abstracts the weird quirks like "return undefined not Promise when you can't hover something".

What's next? Any followup issues?

Provide examples of how to do more complex completions with dependencies:

Before you deploy

I included a bump changeset

The json-languageservice API was a bit awkward to use. Had a bunch of weird type problems that nobody should really have to care about. In this PR, I use an adapter to make folks write Completion & Hover providers in a manner similar to what we do with liquid files. It's not 100% the same, but it's similar enough and abstracts the weird quirks like "return undefined not Promise<undefined> when you can't hover something".

charlespwd · 2024-11-28T19:55:21Z

...language-server-common/src/json/completions/providers/SchemaTranslationCompletionProvider.ts

+  async completeValue(context: RequestContext, path: JSONPath): Promise<JSONCompletionItem[]> {
+    if (!fileMatch(context.doc.uri, this.uriPatterns) || !isLiquidRequestContext(context)) {
+      return [];
+    }
+
+    const { doc, parsed } = context;
+
+    const label = deepGet(parsed, path);
+    if (!label || typeof label !== 'string' || !label.startsWith('t:')) {
+      return [];
+    }
+
+    const partial = /^t:(.*)/.exec(label)?.[1];
+    if (partial === undefined) return [];
+
+    const translations = await this.getDefaultSchemaTranslations(doc.uri);
+
+    // We'll let the frontend do the filtering. But we'll only include shopify
+    // translations if the shopify prefix is present
+    const options = translationOptions(translations);
+
+    return options.map((option): JSONCompletionItem => {
+      const tLabel = `t:${option.path.join('.')}`;
+      return {
+        label: tLabel,
+        kind: CompletionItemKind.Value,
+        filterText: `"${tLabel}"`,
+        insertText: `"${tLabel}"`,
+        insertTextFormat: 1,
+        documentation: {
+          kind: 'markdown',
+          value: renderTranslation(option.translation),
+        },
+      };
+    });
+  }


This is how "easy" I'd like it would be to write a custom JSON completion inside a {% schema %}

charlespwd · 2024-11-28T19:56:15Z

packages/theme-language-server-common/src/json/SchemaTranslationContributions.ts

-  async collectValueCompletions(
-    uri: string,
-    location: JSONPath,
-    propertyKey: string,
-    result: CompletionsCollector,
-  ) {
-    if (!uriMatch(uri, this.uriPatterns)) return;
-    const doc = this.documentManager.get(uri);
-    if (!doc || doc.ast instanceof Error || doc.type !== SourceCodeType.LiquidHtml) {
-      return;
-    }
-
-    const schema = findSchemaNode(doc.ast);
-    if (!schema) return;
-
-    const jsonString = schema.source.slice(
-      schema.blockStartPosition.end,
-      schema.blockEndPosition.start,
-    );
-    const jsonDocument = parseJSON(jsonString);
-    if (!jsonDocument) return;
-
-    const label = location
-      .concat(propertyKey)
-      .reduce((acc: any, val: any) => acc?.[val], jsonDocument);
-    if (!label || typeof label !== 'string' || !label.startsWith('t:')) {
-      return;
-    }
-
-    const items = await this.recommendTranslations(uri, label);
-    for (const item of items) {
-      result.add(item);
-    }
-  }


This thing was split into the SchemaTranslationCompletionProvider

charlespwd · 2024-11-28T19:56:30Z

packages/theme-language-server-common/src/json/SchemaTranslationContributions.ts

-  getInfoContribution(uri: string, location: JSONPath): Promise<string[]> {
-    if (!uriMatch(uri, this.uriPatterns)) return undefined as any;
-    const doc = this.documentManager.get(uri);
-    if (
-      !doc ||
-      location.length === 0 ||
-      doc.ast instanceof Error ||
-      doc.type !== SourceCodeType.LiquidHtml
-    ) {
-      return undefined as any;
-    }
-
-    const schema = findSchemaNode(doc.ast);
-    if (!schema) return undefined as any;
-
-    const jsonString = schema.source.slice(
-      schema.blockStartPosition.end,
-      schema.blockEndPosition.start,
-    );
-    const jsonDocument = parseJSON(jsonString);
-    if (isError(jsonDocument)) return undefined as any;
-
-    const label = location.reduce((acc: any, val: any) => acc?.[val], jsonDocument);
-    if (!label || typeof label !== 'string' || !label.startsWith('t:')) return undefined as any;
-
-    return this.getDefaultSchemaTranslations(uri).then((translations) => {
-      const path = label.slice(2);
-      const value = translationValue(path, translations);
-      if (!value) return undefined as any;
-
-      return [renderTranslation(value)];
-    });
-  }


This thing was split into the SchemaTranslationHoverProvider

charlespwd · 2024-11-28T19:56:54Z

packages/theme-language-server-common/src/json/JSONLanguageService.ts

-              this.documentManager,
-              this.getDefaultSchemaTranslations,
-            ),
+            new JSONContributions(this.documentManager, this.getDefaultSchemaTranslations),


Devs shouldn't need to worry about this anymore other than ferrying new dependencies in there.

aswamy

The rework looks so clean 🧹

Some comments are really for things outside this ticket. Functionally everything works as expected.

.changeset/nice-candles-sparkle.md

aswamy · 2024-11-29T15:47:17Z

...language-server-common/src/json/completions/providers/SchemaTranslationCompletionProvider.ts

+        insertTextFormat: 1,
+        documentation: {
+          kind: 'markdown',
+          value: renderTranslation(option.translation),


Should we render it as markdown in vscode? The links don't work:

The link points to /admin/settings/markets. if you click on it in VSCode, it attempts to create a file.

I like your attention to detail.. but to what store would we make the links to? The extension doesn't have that kind of context right now. Only the CLI does.

I think it's fine to ignore IMO

But yeah maybe we render those as text (not markdown)... idk. I don't feel strongly about either way.

aswamy · 2024-11-29T15:54:59Z

...language-server-common/src/json/completions/providers/SchemaTranslationCompletionProvider.ts

+      const tLabel = `t:${option.path.join('.')}`;
+      return {
+        label: tLabel,


We can probably have this as a low-priority ticket, but we need a better way for the text to overflow when doing translation completion suggestions:

I'd rather see "...blocks.annoucement-bar.name". There really isn't a need to see what we already typed beyond the last previous ..

i.e. to determine the label of the auto-completion, we could do something like:

split the translation path by "."

if you already typed "t:small.piece.of.text" put an "..." between all the parts we already typed and "t:" up to the latest leg of the path: "t:...text"

thing is that search is fuzzy... I feel like that's a client side concern? 🤔

aswamy · 2024-11-29T16:06:31Z

packages/theme-language-server-common/src/json/JSONContributions.ts

+    if (!doc) return SKIP_CONTRIBUTION;
+    const context = this.getContext(doc);
+    const provider = this.hoverProviders.find((p) => p.canHover(context, location));
+    if (!provider) return SKIP_CONTRIBUTION;


I tried to use Promise.reject() instead of SKIP_CONTRIBUTION and it threw an error, but why can't we have Promise.resolve([])?

Because then the contributions that you'd get from the default JSON schema would no longer show up. It's awkward af.

https://github.com/microsoft/vscode-json-languageservice/blob/main/src/services/jsonHover.ts#L56-L59

I use SKIP_CONTRIBUTION because the result is only ignored if you return falsy. And Promise is truthy.

aswamy · 2024-11-29T16:08:53Z

...ages/theme-language-server-common/src/json/hover/providers/SchemaTranslationHoverProvider.ts

+      const path = label.slice(2); // remove `t:`
+      const value = translationValue(path, translations);


Should translationValue just strip the t: from the path if it exists? I feel like we'd be doing this for any new translation providers we make.

I feel like path is clearer than tPathOrPath? I get that it could be nicer to use, but idk how often we'd use this?

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

charlespwd requested a review from aswamy November 28, 2024 19:07

charlespwd force-pushed the feature/block-type-completion-example branch 6 times, most recently from 1d2ce53 to 4377ae2 Compare November 28, 2024 19:39

charlespwd force-pushed the feature/block-type-completion-example branch from 4377ae2 to efe4357 Compare November 28, 2024 19:41

charlespwd commented Nov 28, 2024

View reviewed changes

aswamy approved these changes Nov 29, 2024

View reviewed changes

charlespwd merged commit 05b928e into main Nov 29, 2024
6 checks passed

charlespwd deleted the feature/block-type-completion-example branch November 29, 2024 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the JSON completion & hover building blocks #622

Improve the JSON completion & hover building blocks #622

charlespwd commented Nov 28, 2024 •

edited

Loading

charlespwd Nov 28, 2024

charlespwd Nov 28, 2024

charlespwd Nov 28, 2024

charlespwd Nov 28, 2024

aswamy left a comment

aswamy Nov 29, 2024

charlespwd Nov 29, 2024 •

edited

Loading

charlespwd Nov 29, 2024

aswamy Nov 29, 2024

charlespwd Nov 29, 2024

aswamy Nov 29, 2024

charlespwd Nov 29, 2024

aswamy Nov 29, 2024

charlespwd Nov 29, 2024

		const path = label.slice(2); // remove `t:`
		const value = translationValue(path, translations);

Improve the JSON completion & hover building blocks #622

Improve the JSON completion & hover building blocks #622

Conversation

charlespwd commented Nov 28, 2024 • edited Loading

What are you adding in this PR?

What's next? Any followup issues?

Before you deploy

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aswamy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlespwd Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlespwd commented Nov 28, 2024 •

edited

Loading

charlespwd Nov 29, 2024 •

edited

Loading