Don't register DidChangeWorkspaceFoldersNotification dynamically

[rchl]

Per documentation [1], server announces support for workspace/didChangeWorkspaceFolders
either through workspace.workspaceFolders.supported capability OR through
dynamic registration. Eslint server does both which is wrong [2].
And since eslint server doesn't ever unregister dynamic capability, there is
no point in going that route. Announcing capability from onInitialize is enough.

[1] https://microsoft.github.io/language-server-protocol/specification#workspace_didChangeWorkspaceFolders
[2] https://microsoft.github.io/language-server-protocol/specification#client_registerCapability

[rchl]

@dbaeumer did you have a chance to think about that?

[dbaeumer]

This is actually not correct. The server doesn't register for change notifications in the initialize request. It only announces that it has 'support' for workspace folders.

The server needs to react on change events for workspace folders since it is working directory sensitive hence it needs to flush cashes when workspace folder change.

In general I prefer dynamic registration over static once.

[rchl]

Right, so it's the server capability workspace.workspaceFolders.changeNotifications that actually registers for that notification statically. And vscode-eslint doesn't use that so it has to register capability dynamically. Then it makes sense to me now.

But then this example from VSCode language server API documentation seems wrong because it assumes that it will receive onDidChangeWorkspaceFolders notifications based just on client announcing workspace.workspaceFolders capability. It doesn't itself even announce to support anything workspaceFolder related.

let hasConfigurationCapability: boolean = false;
let hasWorkspaceFolderCapability: boolean = false;
let hasDiagnosticRelatedInformationCapability: boolean = false;

connection.onInitialize((params: InitializeParams) => {
  let capabilities = params.capabilities;

  // Does the client support the `workspace/configuration` request?
  // If not, we will fall back using global settings
  hasConfigurationCapability =
    capabilities.workspace && !!capabilities.workspace.configuration;
  hasWorkspaceFolderCapability =
    capabilities.workspace && !!capabilities.workspace.workspaceFolders;
  hasDiagnosticRelatedInformationCapability =
    capabilities.textDocument &&
    capabilities.textDocument.publishDiagnostics &&
    capabilities.textDocument.publishDiagnostics.relatedInformation;

  return {
    capabilities: {
      textDocumentSync: documents.syncKind,
      // Tell the client that the server supports code completion
      completionProvider: {
        resolveProvider: true
      }
    }
  };
});

connection.onInitialized(() => {
  if (hasConfigurationCapability) {
    // Register for all configuration changes.
    connection.client.register(DidChangeConfigurationNotification.type, undefined);
  }
  if (hasWorkspaceFolderCapability) {
    connection.workspace.onDidChangeWorkspaceFolders(_event => {
      connection.console.log('Workspace folder change event received.');
    });
  }
});

[dbaeumer]

Actually that is correct and the registration can be removed from the ESLint server. But the reason is different than you pointed out. It is because the server implementation does a dynamic registration when onDidChangeWorkspaceFolders is called (see https://github.com/microsoft/vscode-languageserver-node/blob/master/server/src/workspaceFolders.ts#L40)

[dbaeumer]

I am again wrong: the example uses the high level API connection.workspace.onDidChangeWorkspaceFolders which does an automatic registration. The ESLint server uses the low level API

connection.onInitialized(() => {
	connection.client.register(DidChangeConfigurationNotification.type, undefined);
	connection.client.register(DidChangeWorkspaceFoldersNotification.type, undefined);
});

messageQueue.registerNotification(DidChangeConfigurationNotification.type, (_params) => {
	environmentChanged();
});

So both examples are correct.

I will close the PR.