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.

Sign up for GitHub

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

Fix crashes for clients that don't support "workspace/configuration" requests #667

Merged
merged 3 commits into from
Aug 17, 2022
Merged

Fix crashes for clients that don't support "workspace/configuration" requests #667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cf02b68
Fix crash for unsupported workspace/configuration client
TwitchBronBron Aug 17, 2022
1e83b32
Fix another crash related to workspace.getConfiguration
TwitchBronBron Aug 17, 2022
b6f976f
Fix unit test.
TwitchBronBron Aug 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 36 additions & 23 deletions src/LanguageServer.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ describe('LanguageServer', () => {
(server as any).createConnection = () => {
return connection;
};
server['hasConfigurationCapability'] = true;
});
afterEach(async () => {
fsExtra.emptyDirSync(tempDir);
Expand Down Expand Up @@ -388,29 +389,6 @@ describe('LanguageServer', () => {
fsExtra.outputJsonSync(s`${workspacePath}/vendor/someProject/bsconfig.json`, {});
//it always ignores node_modules
fsExtra.outputJsonSync(s`${workspacePath}/node_modules/someProject/bsconfig.json`, {});

await server['syncProjects']();

//no child bsconfig.json files, use the workspacePath
expect(
server.projects.map(x => x.projectPath)
).to.eql([
workspacePath
]);
});

it('ignores bsconfig.json files from vscode ignored paths', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted this totally duplicate unit test

server.run();
sinon.stub(server['connection'].workspace, 'getConfiguration').returns(Promise.resolve({
exclude: {
'**/vendor': true
}
}) as any);

fsExtra.outputJsonSync(s`${workspacePath}/vendor/someProject/bsconfig.json`, {});
//it always ignores node_modules
fsExtra.outputJsonSync(s`${workspacePath}/node_modules/someProject/bsconfig.json`, {});

await server['syncProjects']();

//no child bsconfig.json files, use the workspacePath
Expand Down Expand Up @@ -1003,6 +981,41 @@ describe('LanguageServer', () => {
});
});

describe('getConfigFilePath', () => {
it('honors the hasConfigurationCapability setting', async () => {
server.run();
sinon.stub(server['connection'].workspace, 'getConfiguration').returns(
Promise.reject(
new Error('Client does not support "workspace/configuration"')
)
);
server['hasConfigurationCapability'] = false;
fsExtra.outputFileSync(`${workspacePath}/bsconfig.json`, '{}');
expect(
await server['getConfigFilePath'](workspacePath)
).to.eql(
s`${workspacePath}/bsconfig.json`
);
});
});

describe('getWorkspaceExcludeGlobs', () => {
it('honors the hasConfigurationCapability setting', async () => {
server.run();
sinon.stub(server['connection'].workspace, 'getConfiguration').returns(
Promise.reject(
new Error('Client does not support "workspace/configuration"')
)
);
server['hasConfigurationCapability'] = false;
expect(
await server['getWorkspaceExcludeGlobs'](workspaceFolders[0])
).to.eql([
'**/node_modules/**/*'
]);
});
});

describe('CustomCommands', () => {
describe('TranspileFile', () => {
it('returns pathAbsolute to support backwards compatibility', async () => {
Expand Down
33 changes: 21 additions & 12 deletions src/LanguageServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,14 +230,18 @@ export class LanguageServer {
/**
* Ask the client for the list of `files.exclude` patterns. Useful when determining if we should process a file
*/
private async getWorkspaceExcludeGlobs(workspaceFolder: string) {
//get any `files.exclude` globs to use to filter
let config = await this.connection.workspace.getConfiguration({
scopeUri: workspaceFolder,
section: 'files'
}) as {
exclude: Record<string, boolean>;
private async getWorkspaceExcludeGlobs(workspaceFolder: string): Promise<string[]> {
let config = {
exclude: {} as Record<string, boolean>
};
//if supported, ask vscode for the `files.exclude` configuration
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this comment use the generic "client" instead of specifically mentioning "vscode"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The files and files.exclude are specific to VSCode. I have no idea what other editors to do handle including/excluding files from the workspace, but they're probably doing it differently than this. So, the comment still makes sense, and we can enhance the comment in the future as we add support for other editors.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true that the specific configuration options are not part of the standard. Although there are other clients such as Theia that model their settings after VS Code.

if (this.hasConfigurationCapability) {
//get any `files.exclude` globs to use to filter
config = await this.connection.workspace.getConfiguration({
scopeUri: workspaceFolder,
section: 'files'
});
}
return Object
.keys(config?.exclude ?? {})
.filter(x => config?.exclude?.[x])
Expand Down Expand Up @@ -444,11 +448,16 @@ export class LanguageServer {
} else {
scopeUri = URI.file(workspacePath).toString();
}
//look for config group called "brightscript"
let config = await this.connection.workspace.getConfiguration({
scopeUri: scopeUri,
section: 'brightscript'
});
let config = {
configFile: undefined
};
//if the client supports configuration, look for config group called "brightscript"
if (this.hasConfigurationCapability) {
await this.connection.workspace.getConfiguration({
scopeUri: scopeUri,
section: 'brightscript'
});
}
let configFilePath: string;

//if there's a setting, we need to find the file or show error if it can't be found
Expand Down