Don't send workspace/configuration to clients that don't support it

[benknoble]

yaml-language-server/src/languageserver/handlers/settingsHandlers.ts

Line 41 in 3a74bdc

const config = await this.connection.workspace.getConfiguration([

This function call (AFAICT) sends workspace/configuration, but it is called in response to workspace/didChangeConfiguration:

yaml-language-server/src/languageserver/handlers/settingsHandlers.ts

Line 34 in 3a74bdc

this.connection.onDidChangeConfiguration(() => this.pullConfiguration());

. But that event should already give you the whole configuration!

Worse, the workspace/configuration request is sent to clients that don't support it. For example, I know that ALE (a Vim LSP client) prefers to use the workspace/didChangeConfiguration notification and does not support workspace/configuration, which it indicates by initializing with workspace.configuration capability false.

---

Essentially, this breaks using yaml-language-server with (for example) custom schema configuration because that configuration is never seen by the server.

I don't know if this could be a bug in the vscode-languageserver Node library, but it seems that sending workspace/configuration in response to the workspace/didChangeConfiguration notification is an anti-pattern.

[benknoble]

Here's a patch that fixed this for me. I'm not a typescript developer, so I actually modified the built JS until it worked, then tried to translate back:

diff --git i/src/languageserver/handlers/settingsHandlers.ts w/src/languageserver/handlers/settingsHandlers.ts
index 95ddba7..2fd4e73 100644
--- i/src/languageserver/handlers/settingsHandlers.ts
+++ w/src/languageserver/handlers/settingsHandlers.ts
@@ -14,6 +14,7 @@ import { Telemetry } from '../../languageservice/telemetry';
 import { ValidationHandler } from './validationHandlers';
 
 export class SettingsHandler {
+  private readonly globalSettings: Settings?; // or Settings and initialize to defaults?
   constructor(
     private readonly connection: Connection,
     private readonly languageService: LanguageService,
@@ -31,13 +32,26 @@ export class SettingsHandler {
         this.telemetry.sendError('yaml.settings.error', { error: convertErrorToTelemetryMsg(err) });
       }
     }
-    this.connection.onDidChangeConfiguration(() => this.pullConfiguration());
+    this.connection.onDidChangeConfiguration((event) => {
+      // probably missing some type annotations to make this work
+      if (event && Object.prototype.hasOwnProperty.call(event, 'settings')) {
+        this.globalSettings = event.settings;
+      }
+      this.pullConfiguration()
+    });
   }
 
   /**
    *  The server pull the 'yaml', 'http.proxy', 'http.proxyStrictSSL', '[yaml]' settings sections
    */
   async pullConfiguration(): Promise<void> {
+    // might need a null check on this.globalSettings here; in the JS version I
+    // set globalSettings to {} in the constructor, but that might not pass the
+    // typechecker as a "Settigns" object
+    if (!this.yamlSettings.hasConfigurationCapability) {
+      await this.setConfiguration(this.globalSettings);
+      return;
+    }
     const config = await this.connection.workspace.getConfiguration([
       { section: 'yaml' },
       { section: 'http' },

For reference, here's the modified Javascript that definitely works:

class SettingsHandler {
    constructor(connection, languageService, yamlSettings, validationHandler, telemetry) {
        this.connection = connection;
        this.languageService = languageService;
        this.yamlSettings = yamlSettings;
        this.validationHandler = validationHandler;
        this.telemetry = telemetry;
        this.globalSettings = {};
    }
    async registerHandlers() {
        if (this.yamlSettings.hasConfigurationCapability && this.yamlSettings.clientDynamicRegisterSupport) {
            try {
                // Register for all configuration changes.
                await this.connection.client.register(vscode_languageserver_1.DidChangeConfigurationNotification.type);
            }
            catch (err) {
                this.telemetry.sendError('yaml.settings.error', { error: (0, objects_1.convertErrorToTelemetryMsg)(err) });
            }
        }
        this.connection.onDidChangeConfiguration((event) => {
          if (event && Object.prototype.hasOwnProperty.call(event, 'settings')) {
            this.globalSettings = event.settings;
          }
          this.pullConfiguration()
        });
    }
    /**
     *  The server pull the 'yaml', 'http.proxy', 'http.proxyStrictSSL', '[yaml]' settings sections
     */
    async pullConfiguration() {
      if (!this.yamlSettings.hasConfigurationCapability) {
        await this.setConfiguration(this.globalSettings);
        return;
      }
// snip: omitted