Shopify Liquid VS Code extension for Web #529

charlespwd · 2024-10-08T18:25:10Z

What are you adding in this PR?

Major breaking changes in most packages (mostly internal APIs)
- vscode-extension:
  - We now have a browser entry in our package.json
  - The src folder has been split into node, common and browser folders
  - The browser module starts the language server in a Web Worker
  - The node module starts the language server in a subprocess
  - Dropped support for CLI based language servers (legacy extension)
  - There's two new ways to test/debug the VS Code extension in browser contexts (see tophatting section below).
- theme-check-*, and theme-language-server-*:
  - Replace AbsolutePath concerns with Uris.
    - Absolute paths don't make sense in browser
    - Absolute paths don't make sense with remote files
- Replace all "fs-based" Dependencies with a single fs: AbstractFileSystem dependency
  - The old injections are now common implementations based on the abstract fs
  - For the CLI theme check, a NodeFileSystem is available (and used by default)
  - For the CLI language server, a NodeFileSystem is available (and used by default)
  - For the VS Code extension, a VsCodeFileSystem is available
    - This file system implementation queries the VS Code FileSystem API (only available in the extension host thread) for file contents. This query is done on the language server via non-standard LSP messages:
      - fs/readFile -> a TextDecoded vscode.workspace.fs.readFile(uri)
      - fs/readDirectory -> vscode.workspace.fs.readDirectory(uri)
      - fs/stat -> vscode.workspace.fs.stat(uri)
    - In node, the same happens but short circuits to the NodeFileSystem for file:/// URIs.
      - We don't need to send file contents over STDIO when we can query the FS directly
- The codemirror-language-client's playground implements a mock "MainThreadFileSystem" that we could use as inspiration for online-store-web

Demo

vscode-for-web-vfs.mp4

Top-hatting

There's 2 new ways you can do this. They are documented in docs/contributing. But here's a video.

Debugging in Chromium VS Code for Web

The "true" E2E way. Demo worthy version of "showing it works in browser"

vscode-for-web-vfs-2.mp4

Debugging in the Desktop app

A faster, debuggable way that runs the browser extension in the desktop app. This disables all the Node APIs but it lets you debug the code.

Debugging the CodeMirror Language client

(Not new, but you should know that this flow is also updated to virtual file systems).

TODO

What's next? Any followup issues?

Make loadConfig work in-browser
Make the prettier plugin work in-browser

What did you learn?

A couple of neat things that the vscode libraries already offer.
- The web worker stuff is offered via BrowserMessageReader(worker) and BrowserMessageWriter(worker).
- vscode.workspace.fs is pretty cool. It does a pretty clever the URIScheme -> FileSystemProvider mapping that makes it so we could eventually have a "ShopifyBackendFileSystem" that would write directly to the storefront.

Before you deploy

I included a bunch major bump changeset

charlespwd · 2024-10-08T18:26:45Z

packages/theme-language-server-browser/src/index.ts

-  };
-
+  const reader = new BrowserMessageReader(worker);
+  const writer = new BrowserMessageWriter(worker);


I learned by looking at examples that there was an easier way :)

charlespwd · 2024-10-08T18:27:17Z

packages/vscode-extension/package.json

@@ -40,6 +40,7 @@
    "dev:build": "webpack --mode development",
    "dev:syntax": "yarn --cwd ./syntaxes dev",
    "dev:watch": "webpack --mode development --watch",
+    "dev:web": "yarn build && vscode-test-web --permission=clipboard-read --permission=clipboard-write --browserType=chromium --extensionDevelopmentPath=.",


vscode-test-web is the one that lets you run chrome and have vscode run in it with your extension loaded. It was a real breeze ngl!

charlespwd · 2024-10-08T18:27:45Z

packages/vscode-extension/package.json

@@ -83,7 +85,8 @@
  "activationEvents": [
    "workspaceContains:**/.theme-check.yml"
  ],
-  "main": "./dist/extension.js",
+  "main": "./dist/node/extension.js",
+  "browser": "./dist/browser/extension.js",


Major difference is here: We have a different build for web that uses theme-language-server-browser instead of theme-language-server-node

charlespwd · 2024-10-08T18:28:18Z

packages/vscode-extension/package.json

+            "messages",
+            "verbose"
+          ]
+        },


Added that stuff in there because I was tired of not having code completion in settings.json to enable the LSP logging

charlespwd · 2024-10-08T18:28:58Z

packages/vscode-extension/src/browser/extension.ts

+function createWorkerLanguageClient(
+  context: ExtensionContext,
+  clientOptions: LanguageClientOptions,
+) {
+  // Create a worker. The worker main file implements the language server.
+  const serverMain = Uri.joinPath(context.extensionUri, 'dist', 'browser', 'server.js');
+  const worker = new Worker(serverMain.toString(true));
+
+  // create the language server client to communicate with the server running in the worker
+  return new LanguageClient(
+    'shopifyLiquid',
+    'Theme Check Language Server',
+    clientOptions,
+    worker,
+  );
+}


Major difference is that here we start the language server in a Web Worker instead of settingup STDIO and a subprocess.

packages/vscode-extension/src/browser/server.ts

packages/vscode-extension/src/node/extension.ts

- Change `Config` interface to refer to `rootUri` instead of `root` as an absolutePath - Get rid of `fileExists` DI in theme-check Dependencies - Get rid of `fileExists` DI in theme-language-server Dependencies - Add `fs` DI in theme-check Dependencies - Add `fs` DI in theme-language-server Dependencies - Add `MockFileSystem` implementation (for tests) - Add `NodeFileSystem` implementation - Add optional `fs` argument to `theme-language-server-node#startServer()` The `fs` DI has the following interface (so far): ```ts interface FileSystem { stat(uri: string): Promise<{ fileType, size }>; readFile(uri: string): Promise<string>; readDirectory(uri: string): Promise<[uri: string, fileType: FileType][]>; } ``` theme-language-server-node now also accepts an optional `fs` implementation in startServer.

Instead of having that logic be injected in, I added an abstract implementation in theme-check-common that depends on the virtual file system. We will no longer need to inject this behaviour :)

FileSystem is an ambient import which made the DX suffer when refactoring code. It's also more obvious now that the FileSystem interface is an interface that you need to implement, not something concrete that you can use directly.

Replace it with URIs. That way we won't need to worry about windows paths being weird. The only place where we still have them is for places where we deal with `glob` and `loadConfig` in `theme-{check,language-server}-node`. - SourceCode.absolutePath -> SourceCode.uri - etc.

We still have an optiomal dependency that is otherwise implemented by a naive implementation (for CLI use cases), but in the language server we prefer the buffer over the file in the file system... Which makes me think we could get rid of that by actually making that logic part of the getTranslationsFactory directly. Prefer theme files over fs.

All that logic didn't need to be in the language server. This makes the dep injection easier too because we no longer have this upstream concern.

Replace with AbstractFileSystem-based implementation

Replace with abstract-file-system-based implementation TODO make that shit faster. Seems highlighy unoptimized.

Replace with abstract-file-system-based implementation in common code.

- Adds support for VS Code for Web - Adds support for remote files

You don't need to query VS Code _every time_ the user types something for the same imformation. That's rather slow! We'll invalidate the relevant caches when files are saved/deleted/created/renamed.

charlespwd · 2024-10-17T15:53:47Z

packages/theme-check-common/src/AbstractFileSystem.ts

+/**
+ * The AbstractFileSystem interface is used to abstract file system operations.
+ *
+ * This way, the Theme Check library can be used in different environments,
+ * such as the browser, node.js or VS Code (which works with local files, remote
+ * files and on the web)
+ */
+export interface AbstractFileSystem {
+  stat(uri: string): Promise<FileStat>;
+  readFile(uri: string): Promise<string>;
+  readDirectory(uri: string): Promise<FileTuple[]>;
+}
+
+export enum FileType {
+  Unknown = 0,
+  File = 1,
+  Directory = 2,
+  SymbolicLink = 64,
+}
+
+export interface FileStat {
+  type: FileType;
+  size: number;
+}
+
+/** A vscode-uri string */
+export type UriString = string;
+
+export type FileTuple = [uri: UriString, fileType: FileType];


Probably the most important file in this large diff.

charlespwd · 2024-10-17T15:56:26Z

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

-    fileExists,
-    fileSize,
-    filesForURI,
-    findRootURI: findConfigurationRootURI,
-    getDefaultLocaleFactory,
-    getDefaultTranslationsFactory,
-    getDefaultSchemaLocaleFactory,
-    getDefaultSchemaTranslationsFactory,
-    getThemeSettingsSchemaForRootURI,


charlespwd · 2024-10-17T15:56:42Z

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

-      fileExists,
-      fileSize,
-      findRootURI: findConfigurationRootURI,
-      getDefaultLocaleFactory,
-      getDefaultTranslationsFactory,
-      getDefaultSchemaLocaleFactory,
-      getDefaultSchemaTranslationsFactory,
+      fs,


charlespwd · 2024-10-17T15:58:18Z

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

@@ -48,7 +51,7 @@ export interface RequiredDependencies {
   * @param uri - a file path
   * @returns {Promise<Config>}
   */
-  loadConfig(uri: URI): Promise<Config>;
+  loadConfig(uri: URI, fileExists: (uri: string) => Promise<boolean>): Promise<Config>;


the fileExists here is a performance improvement. Wanted loadConfig to use findRoot and that thing depends on fileExists which should be the CachedFileSystem, not the raw one.

charlespwd · 2024-10-17T15:59:44Z

packages/vscode-extension/src/node/extension.ts

-      { scheme: 'file', language: 'liquid' },
-      { scheme: 'file', language: 'plaintext' },
-      { scheme: 'file', language: 'html' },
-      { scheme: 'file', language: 'javascript' },
-      { scheme: 'file', language: 'css' },
-      { scheme: 'file', language: 'scss' },
-      { scheme: 'file', language: 'json' },
-      { scheme: 'file', language: 'jsonc' },
+      { language: 'liquid' },
+      { language: 'plaintext' },
+      { language: 'html' },
+      { language: 'javascript' },
+      { language: 'css' },
+      { language: 'scss' },
+      { language: 'json' },
+      { language: 'jsonc' },


This thing took me a while to figure out since I wrote this 3 years ago. We need to remove scheme in order for completion requests from vscode-vfs:// URIs to trigger.

charlespwd · 2024-10-17T16:01:23Z

packages/vscode-extension/webpack.config.js

+      libraryTarget: 'var',
+      library: 'serverExportVar',
+      devtoolModuleFilenameTemplate: '../../[resource-path]',


The only reason I need 2x a config that should look like one is because of this difference here. The worker has to be "import-less", but the extension needs a require('vscode') call in the output that gets shimmed.

Made to demo what it's like to use a VirtualFileSystem in a non-vscode environment.

charlespwd commented Oct 8, 2024

View reviewed changes

packages/vscode-extension/src/browser/server.ts Outdated Show resolved Hide resolved

charlespwd commented Oct 8, 2024

View reviewed changes

packages/vscode-extension/src/node/extension.ts Outdated Show resolved Hide resolved

charlespwd force-pushed the feature/521-virtual-file-system branch 2 times, most recently from 7cb9d21 to ccf72aa Compare October 9, 2024 13:46

Simplify theme-language-server-browser connection creation

d1ad293

charlespwd force-pushed the feature/521-virtual-file-system branch from 71a2c6b to 2ffbef6 Compare October 9, 2024 14:05

charlespwd added 3 commits October 9, 2024 10:09

Add VS Code for Web Extension skeleton implementation

8ae71e3

Remove support for ruby-based language server

253983d

Add VS Code for Web major bump changeset

4d7cefe

charlespwd force-pushed the feature/521-virtual-file-system branch from 2ffbef6 to 4d7cefe Compare October 9, 2024 14:09

charlespwd added 3 commits October 10, 2024 15:41

Fixup the sourcemaps & launch.json for dual build

9feef3b

Clean up asset-size-based tests

3ee1903

Include theme check test helpers in dist

54b290e

charlespwd force-pushed the feature/521-virtual-file-system branch from c22727f to e1aed1e Compare October 10, 2024 20:06

charlespwd added 2 commits October 15, 2024 09:42

Get rid of the fileSize injection (use abstract fs.stat instead)

f0e085c

charlespwd force-pushed the feature/521-virtual-file-system branch from c9b4e00 to 7bd1486 Compare October 15, 2024 13:42

charlespwd added 2 commits October 15, 2024 12:21

Replace getDefault(Schema)?Locale dependency injection

74eddea

Instead of having that logic be injected in, I added an abstract implementation in theme-check-common that depends on the virtual file system. We will no longer need to inject this behaviour :)

Rename FileSystem to AbstractFileSystem

35ff79e

FileSystem is an ambient import which made the DX suffer when refactoring code. It's also more obvious now that the FileSystem interface is an interface that you need to implement, not something concrete that you can use directly.

charlespwd force-pushed the feature/521-virtual-file-system branch 2 times, most recently from 4f24bdc to ae0ac49 Compare October 15, 2024 17:29

charlespwd added 2 commits October 15, 2024 13:31

Fixup baseUrl in tcn

2e4bf7d

charlespwd force-pushed the feature/521-virtual-file-system branch 2 times, most recently from 07b17c9 to dd53f1a Compare October 15, 2024 17:37

charlespwd added 2 commits October 15, 2024 15:48

Uri->UriString for clarity

28dfbe8

charlespwd force-pushed the feature/521-virtual-file-system branch from 39b4dbe to 28c2318 Compare October 16, 2024 14:33

charlespwd added 3 commits October 16, 2024 10:54

Move buffer translations concern to theme-check-common

c0980f9

All that logic didn't need to be in the language server. This makes the dep injection easier too because we no longer have this upstream concern.

Remove filesForURI language server dependency

6f6db6b

Replace with AbstractFileSystem-based implementation

Remove getThemeSettingsSchemaForURI injection

6ec9236

Replace with abstract-file-system-based implementation TODO make that shit faster. Seems highlighy unoptimized.

charlespwd force-pushed the feature/521-virtual-file-system branch from 28c2318 to 6ec9236 Compare October 16, 2024 14:54

charlespwd added 2 commits October 16, 2024 11:16

Remove findRootURI injection

a501a10

Replace with abstract-file-system-based implementation in common code.

Implement and setup basic VsCodeFileSystem

71d5086

- Adds support for VS Code for Web - Adds support for remote files

charlespwd force-pushed the feature/521-virtual-file-system branch from 7c5b372 to 71d5086 Compare October 16, 2024 19:50

Optimize FS calls in the Language Server

a15e42c

You don't need to query VS Code _every time_ the user types something for the same imformation. That's rather slow! We'll invalidate the relevant caches when files are saved/deleted/created/renamed.

charlespwd commented Oct 17, 2024

View reviewed changes

Implement MainThreadFileSystem in CodeMirror Playground

01e1f06

Made to demo what it's like to use a VirtualFileSystem in a non-vscode environment.

charlespwd force-pushed the feature/521-virtual-file-system branch 2 times, most recently from 3919133 to 8a05d45 Compare October 17, 2024 18:05

Add a bunch of changesets

f3d1197

charlespwd force-pushed the feature/521-virtual-file-system branch from 8a05d45 to f3d1197 Compare October 17, 2024 18:06

charlespwd mentioned this pull request Oct 17, 2024

Unused files check #518

Open

Docs changes and webpack config cleanup

3a7ea94

charlespwd force-pushed the feature/521-virtual-file-system branch from 95da0b8 to 3a7ea94 Compare October 18, 2024 14:45

charlespwd requested review from aswamy and graygilmore October 18, 2024 15:04

charlespwd marked this pull request as ready for review October 18, 2024 15:04

charlespwd added 2 commits October 18, 2024 11:06

Add toplevel yarn dev:web command

d87c592

Minor cleanups

0a58241

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shopify Liquid VS Code extension for Web #529

Shopify Liquid VS Code extension for Web #529

charlespwd commented Oct 8, 2024 •

edited

Loading

charlespwd Oct 8, 2024

charlespwd Oct 8, 2024

charlespwd Oct 8, 2024 •

edited

Loading

charlespwd Oct 8, 2024

charlespwd Oct 8, 2024

charlespwd Oct 17, 2024

charlespwd Oct 17, 2024

charlespwd Oct 17, 2024

charlespwd Oct 17, 2024

charlespwd Oct 17, 2024

charlespwd Oct 17, 2024

Shopify Liquid VS Code extension for Web #529

Are you sure you want to change the base?

Shopify Liquid VS Code extension for Web #529

Conversation

charlespwd commented Oct 8, 2024 • edited Loading

What are you adding in this PR?

Demo

Top-hatting

Debugging in Chromium VS Code for Web

Debugging in the Desktop app

Debugging the CodeMirror Language client

What's next? Any followup issues?

What did you learn?

Before you deploy

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlespwd Oct 8, 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

Choose a reason for hiding this comment

charlespwd commented Oct 8, 2024 •

edited

Loading

charlespwd Oct 8, 2024 •

edited

Loading