From 3effb5eab02a1e3791a9262fdc79e7c9c257ba0f Mon Sep 17 00:00:00 2001 From: Bronley Plumb Date: Thu, 26 Sep 2024 14:26:14 -0400 Subject: [PATCH 1/3] Fix crashes where workspaceFolders was undefined --- src/BrightScriptCommands.ts | 4 ++-- src/DebugConfigurationProvider.ts | 2 +- src/DeclarationProvider.ts | 2 +- src/LanguageServerManager.ts | 4 ++-- src/commands/LanguageServerInfoCommand.ts | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/BrightScriptCommands.ts b/src/BrightScriptCommands.ts index c81e722a..65a8e963 100644 --- a/src/BrightScriptCommands.ts +++ b/src/BrightScriptCommands.ts @@ -454,8 +454,8 @@ export class BrightScriptCommands { this.workspacePath = await this.context.workspaceState.get('workspacePath'); //let folderUri: vscode.Uri; if (!this.workspacePath) { - if (vscode.workspace.workspaceFolders.length === 1) { - this.workspacePath = vscode.workspace.workspaceFolders[0].uri.fsPath; + if (vscode.workspace.workspaceFolders?.length === 1) { + this.workspacePath = vscode.workspace.workspaceFolders?.[0].uri.fsPath; } else { //there are multiple workspaces, ask the user to specify which one they want to use let workspaceFolder = await vscode.window.showWorkspaceFolderPick(); diff --git a/src/DebugConfigurationProvider.ts b/src/DebugConfigurationProvider.ts index e7de2cb3..26147add 100644 --- a/src/DebugConfigurationProvider.ts +++ b/src/DebugConfigurationProvider.ts @@ -234,7 +234,7 @@ export class BrightScriptDebugConfigurationProvider implements DebugConfiguratio folderUri = folder.uri; //if there's only one workspace, use that workspace's folder path - } else if (vscode.workspace.workspaceFolders.length === 1) { + } else if (vscode.workspace.workspaceFolders?.length === 1) { folderUri = vscode.workspace.workspaceFolders[0].uri; } else { //there are multiple workspaces, ask the user to specify which one they want to use diff --git a/src/DeclarationProvider.ts b/src/DeclarationProvider.ts index 9cc94ddd..e1b3979c 100644 --- a/src/DeclarationProvider.ts +++ b/src/DeclarationProvider.ts @@ -49,7 +49,7 @@ export class WorkspaceEncoding { public reset() { this.encoding = []; - for (const folder of vscode.workspace.workspaceFolders) { + for (const folder of vscode.workspace.workspaceFolders ?? []) { this.encoding.push([folder.uri.fsPath, this.getConfiguration(folder.uri)]); } } diff --git a/src/LanguageServerManager.ts b/src/LanguageServerManager.ts index 69449d64..ffa6041c 100644 --- a/src/LanguageServerManager.ts +++ b/src/LanguageServerManager.ts @@ -468,14 +468,14 @@ export class LanguageServerManager { } //collect `brightscript.bsdk` setting value from each workspaceFolder - const folderResults = vscode.workspace.workspaceFolders.reduce((acc, workspaceFolder) => { + const folderResults = vscode.workspace.workspaceFolders?.reduce((acc, workspaceFolder) => { const versionInfo = vscode.workspace.getConfiguration('brightscript', workspaceFolder).get('bsdk'); const parsed = this.parseVersionInfo(versionInfo, workspaceFolder.uri.fsPath); if (parsed) { acc.set(parsed.value, parsed); } return acc; - }, new Map()); + }, new Map()) ?? new Map(); //no results found, use the embedded version if (folderResults.size === 0) { diff --git a/src/commands/LanguageServerInfoCommand.ts b/src/commands/LanguageServerInfoCommand.ts index 59060393..44311adb 100644 --- a/src/commands/LanguageServerInfoCommand.ts +++ b/src/commands/LanguageServerInfoCommand.ts @@ -141,7 +141,7 @@ export class LanguageServerInfoCommand { */ public async selectBrighterScriptVersion(): Promise { const quickPickItems = this.discoverBrighterScriptVersions( - vscode.workspace.workspaceFolders.map(x => this.getWorkspaceOrFolderPath(x.uri.fsPath)) + vscode.workspace.workspaceFolders?.map(x => this.getWorkspaceOrFolderPath(x.uri.fsPath)) ?? [] ); //start the request right now, we will leverage it later From 6ca439a3168efd3bd52c4bd72207bd0196569a69 Mon Sep 17 00:00:00 2001 From: Bronley Plumb Date: Thu, 26 Sep 2024 15:01:32 -0400 Subject: [PATCH 2/3] Fix fallback to built-in lsp when bsdk cannot be resolved --- src/LanguageServerManager.spec.ts | 22 +++++++++++++++++++++ src/LanguageServerManager.ts | 8 ++++++-- src/testHelpers.spec.ts | 32 +++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 2 deletions(-) create mode 100644 src/testHelpers.spec.ts diff --git a/src/LanguageServerManager.spec.ts b/src/LanguageServerManager.spec.ts index d6f78716..2c27fcd2 100644 --- a/src/LanguageServerManager.spec.ts +++ b/src/LanguageServerManager.spec.ts @@ -18,6 +18,7 @@ import { import { util } from './util'; import { GlobalStateManager } from './GlobalStateManager'; import { LocalPackageManager } from './managers/LocalPackageManager'; +import { expectThrowsAsync } from './testHelpers.spec'; const Module = require('module'); const sinon = createSandbox(); @@ -345,6 +346,27 @@ describe('LanguageServerManager', () => { //these tests take a long time (due to running `npm install`) this.timeout(5 * 60 * 1000); + it('returns info when absolute dir already exists', async () => { + const versionInfo = s`${tempDir}/node_modules/brighterscript`; + fsExtra.outputJsonSync(s`${tempDir}/node_modules/brighterscript/package.json`, { + version: '0.65.0' + }); + expect( + await languageServerManager['ensureBscVersionInstalled'](versionInfo) + ).to.eql({ + packageDir: s`${tempDir}/node_modules/brighterscript`, + versionInfo: versionInfo, + version: '0.65.0' + }); + }); + + it('throws when dir does not exist', async () => { + const versionInfo = s`${tempDir}/node_modules/brighterscript`; + await expectThrowsAsync(async () => { + await languageServerManager['ensureBscVersionInstalled'](versionInfo); + }, `"${s(versionInfo)}" does not contain a package.json file`); + }); + it('installs a bsc version when not present', async () => { const info = await languageServerManager['ensureBscVersionInstalled']('0.65.0'); expect(info).to.eql({ diff --git a/src/LanguageServerManager.ts b/src/LanguageServerManager.ts index ffa6041c..c4e65560 100644 --- a/src/LanguageServerManager.ts +++ b/src/LanguageServerManager.ts @@ -439,7 +439,7 @@ export class LanguageServerManager { if (versionInfo === 'embedded') { return { type: 'dir', - value: this.embeddedBscInfo.packageDir + value: s`${this.embeddedBscInfo.packageDir}` }; } else { return this.localPackageManager.parseVersionInfo(versionInfo, cwd); @@ -513,8 +513,12 @@ export class LanguageServerManager { //if this is a directory, use it as-is if (parsed.type === 'dir') { + //if the directory does not exist, throw an error + if (await fsExtra.pathExists(s`${parsed.value}/package.json`) === false) { + throw new Error(`"${parsed.value}" does not contain a package.json file`); + } return { - packageDir: parsed.value, + packageDir: s`${parsed.value}`, version: fsExtra.readJsonSync(s`${parsed.value}/package.json`, { throws: false })?.version ?? parsed.value, versionInfo: versionInfo }; diff --git a/src/testHelpers.spec.ts b/src/testHelpers.spec.ts new file mode 100644 index 00000000..cacdf7be --- /dev/null +++ b/src/testHelpers.spec.ts @@ -0,0 +1,32 @@ +import { expect } from 'chai'; + +export function expectThrows(callback: () => any, expectedMessage: string | undefined = undefined, failedTestMessage = 'Expected to throw but did not') { + let wasExceptionThrown = false; + try { + callback(); + } catch (e) { + wasExceptionThrown = true; + if (expectedMessage) { + expect(e.message).to.eql(expectedMessage); + } + } + if (wasExceptionThrown === false) { + throw new Error(failedTestMessage); + } +} + + +export async function expectThrowsAsync(callback: () => any, expectedMessage: string | undefined = undefined, failedTestMessage = 'Expected to throw but did not') { + let wasExceptionThrown = false; + try { + await Promise.resolve(callback()); + } catch (e) { + wasExceptionThrown = true; + if (expectedMessage) { + expect(e.message).to.eql(expectedMessage); + } + } + if (wasExceptionThrown === false) { + throw new Error(failedTestMessage); + } +} From 1c6dcf4917b332c3f5301a023867422d87c19e35 Mon Sep 17 00:00:00 2001 From: Bronley Plumb Date: Thu, 26 Sep 2024 16:06:38 -0400 Subject: [PATCH 3/3] Better error message wording --- src/LanguageServerManager.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/LanguageServerManager.ts b/src/LanguageServerManager.ts index c4e65560..1fcda1fe 100644 --- a/src/LanguageServerManager.ts +++ b/src/LanguageServerManager.ts @@ -423,8 +423,8 @@ export class LanguageServerManager { this.selectedBscInfo = await this.ensureBscVersionInstalled(versionInfo); } catch (e) { console.error(e); - //fall back to the embedded version, and show a popup - await vscode.window.showErrorMessage(`Can't find language server for "${versionInfo}". Did you forget to run \`npm install\`? Using embedded version v${this.embeddedBscInfo.version} instead.`); + //fall back to the embedded version, and show a popup (don't await the popup because that blocks this flow) + void vscode.window.showErrorMessage(`Language server failure. Did you forget \`npm install\`? Using embedded version ${this.embeddedBscInfo.version}. Can't find language server for "${versionInfo}"`); this.selectedBscInfo = this.embeddedBscInfo; }