Skip to content
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

🐛 Extension doesn't reload when installed Biome version changes #173

Closed
1 of 3 tasks
selesse opened this issue Mar 20, 2024 · 1 comment
Closed
1 of 3 tasks

🐛 Extension doesn't reload when installed Biome version changes #173

selesse opened this issue Mar 20, 2024 · 1 comment
Labels
Enhancement Improves an existing feature

Comments

@selesse
Copy link

selesse commented Mar 20, 2024

VS Code version

1.87.2

Extension version

2.2.2

Biome version

1.6.1

Operating system

  • Windows
  • macOS
  • Linux

Description

TL;DR: We could make Biome-VSCode's version reloading more reliable (to build upon #91) by using the VS Code file system watcher to monitor changes in node_modules/@biomejs/biome. The current createFileSystemWatcher doesn't support looking at user excluded folders (e.g. node_modules/**/*) even though its documentation implies it can, but a proposed API change (microsoft/vscode#169724) would allow us to do this.


When the installed version of Biome changes, the VS Code extension doesn't reload the Biome version. In #91, the
extension started listening for changes to any lockfile to determine when to reload. While that solves part of the problem, it does not account for a common workflow: one person updates the Biome version, checks in the updated version and lockfile, and another person pulls those changes.

Specifically, using arbitrary version numbers and Bun to help illustrate the issue, if we:

  1. Have a package.json locked to biome version 1.5.3, bun install, touch bun.lockb, the biome-vscode extension will load Biome version 1.5.3
  2. In an untracked folder, make a commit where biome is bumped to 1.6.1, run bun install, include the commit of the lockfile being updated
  3. Pull that commit. The lockfile will be updated, the biome-vscode extension will attempt a reload but nothing will change because locally we're still running 1.5.3
  4. Run bun/yarn/npm install. The biome version will be updated to 1.6.1, but since the lockfile won't change (it was already pulled in), the extension won't reload it.

I believe this issue would also be present the first time cloning a repo, since only monitoring the lockfile for changes would result in us triggering a reload before Biome's installed.


The ideal fix for this (IMO) is to add a watcher on the node_modules/@biomejs folder to look to detect when the version changes (for example, node_modules/@biomejs/biome/package.json. In #91, we specifically avoided doing that because the node_modules folder is excluded from VS Code:

biome-vscode/src/main.ts

Lines 185 to 186 in a8da209

// Best way to determine package updates. Will work for npm, yarn, pnpm and bun. (Might work for more files also).
// It is not possible to listen node_modules, because it is usually gitignored.

Digging into that, I noticed that the documentation of the createFileSystemWatcher heavily implies that it's possible to create a non-recursive watcher in a user excluded folder: https://github.com/microsoft/vscode/blob/1.87.2/src/vscode-dts/vscode.d.ts#L13182-L13186. This comment also implies it should be possible.

Unfortunately, I could not figure out any way to get VS Code to watch this excluded folder. Here's a patch that I thought would work based on the documentation:

diff --git a/src/main.ts b/src/main.ts
index 7d512a0..cd2a6f9 100644
--- a/src/main.ts
+++ b/src/main.ts
@@ -184,27 +184,34 @@ export async function activate(context: ExtensionContext) {
 	if (workspace.workspaceFolders?.[0]) {
 		// Best way to determine package updates. Will work for npm, yarn, pnpm and bun. (Might work for more files also).
-  		// It is not possible to listen node_modules, because it is usually gitignored.
-		const watcher = workspace.createFileSystemWatcher(
+		const lockfileWatcher = workspace.createFileSystemWatcher(
 			new RelativePattern(workspace.workspaceFolders[0], "*lock*"),
 		);
+		const biomeVersionWatcher = workspace.createFileSystemWatcher(
+			new RelativePattern(workspace.workspaceFolders[0], 'node_modules/@biomejs/biome/package.json'),
+		);
+
+		async function reloadBiomeExecutable() {
+			try {
+				outputChannel.appendLine("Reloading biome executable.");
+				if (client.isRunning()) {
+					await client.stop();
+				}
+				await reloadClient();
+				if (client.isRunning()) {
+					await client.restart();
+				} else {
+					await client.start();
+				}
+			} catch (error) {
+				outputChannel.appendLine(`Reloading client failed: ${error}`);
+			}
+		}
+		// When the lockfile OR the installed biome version changes, reload the biome executable.
 		context.subscriptions.push(
-			watcher.onDidChange(async () => {
-				try {
-					// When the lockfile changes, reload the biome executable.
-					outputChannel.appendLine("Reloading biome executable.");
-					if (client.isRunning()) {
-						await client.stop();
-					}
-					await reloadClient();
-					if (client.isRunning()) {
-						await client.restart();
-					} else {
-						await client.start();
-					}
-				} catch (error) {
-					outputChannel.appendLine(`Reloading client failed: ${error}`);
-				}
-			}),
+			lockfileWatcher.onDidChange(reloadBiomeExecutable),
+			biomeVersionWatcher.onDidChange(reloadBiomeExecutable),
+			biomeVersionWatcher.onDidCreate(reloadBiomeExecutable),
 		);
 	}
 

There is a new proposed API for createFileSystemWatcher that solves this problem by accepting an excludes option to be passed:

const biomeVersionWatcher = workspace.createFileSystemWatcher(
	new RelativePattern(workspace.workspaceFolders[0], 'node_modules/@biomejs/biome/package.json'),
	{ excludes: [] },
);

However, since it's a proposed API, it only works in development with code . --enable-proposed-api biomejs.biome and adding "enabledApiProposals": ["createFileSystemWatcher"] to the package.json.

I would recommend waiting until the proposed API gets merged and using this patch to have better version swapping support.

Steps to reproduce

See above.

Expected behavior

See above.

Does this issue occur when using the CLI directly?

No

Logs

No response

@selesse selesse added the Triage label Mar 20, 2024
@nhedger nhedger added Enhancement Improves an existing feature and removed Triage labels Apr 1, 2024
@nhedger
Copy link
Member

nhedger commented Aug 22, 2024

AFAIK this has already been implemented.

@nhedger nhedger closed this as completed Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improves an existing feature
Projects
None yet
Development

No branches or pull requests

2 participants