onNotification can only have one event handler per type - consider allowing multiple?

[CRogers]

I spent a large chunk of time today tracking down a bug I'd introduced; it looks like LanguageClient#onNotification can only have one handler per notification type, and silently overwrites the old handler when given a new one:

https://github.com/Microsoft/vscode-languageserver-node/blob/fd152c3961eea1a74547932d157d7351764c1dbf/jsonrpc/src/main.ts#L898

This goes very much against what I'd expect to be able to do, which is to register multiple handlers for the same notification type.

In particular, I'm trying to add vscode decorations for haskell "typed holes" which are hacked into the haskell compiler as error messages but need a faster feedback than hovering the mouse:

I want to augment the existing error reporting provided by the default vscode language client implementation but can't do it without removing that default publish diagnostic handler at the moment :/

[dbaeumer]

The problem here is that although this is possible for notifications it is not for requests (due to the response). To keep things consistent I made them 'singletons' for both notifications and requests and called them handlers and not event listeners.

I am reluctant to change this for handlers since it makes things inconsistent. Instead we should route the diagnostic notification through the client middleware so that the result can be inspected there and further action can be taken. We route all the request through the middleware as well for that purpose.

@CRogers are you willing to look into a PR for this?

[CRogers]

I'm happy to give it a go if we can work out what the change to the middleware api would be.

…

On Mon, 8 Jan 2018, 10:57 Dirk Bäumer, ***@***.***> wrote: The problem here is that although this is possible for notifications it is not for requests (do to the response). To keep things consistent I made them 'singletons' for both notifications and requests and called them handlers and not event listeners. I am reluctant to change this for handlers since it makes things inconsistent. Instead we should route the diagnostic notification through the client middleware so that the result can be inspected there and further action can be taken. We route all the request through the middleware as well for that purpose. @CRogers <https://github.com/crogers> are you willing to look into a PR for this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#299 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYR4xekmQchiqDEwTdGBpeHvx0UBoRpks5tIfSPgaJpZM4RRLkO> .

[dbaeumer]

The middle ware is defined in client.ts: https://github.com/Microsoft/vscode-languageserver-node/blob/master/client\src\client.ts#L399

We should add a signature for publishDiagnostics with the types from the LSP (since the message comes from the server to intercept it as early as possible). The code that currently published the diagnostics should pass them to the middleware if it provides a special handler and should pass the current publish code in the next handler.

[rcjsuen]

I've opened #322 to address this.

[rcjsuen]

@CRogers #322 has been merged so there should be a new middleware handleDiagnostics function available for you to inspect the diagnostics before VS Code "knows" about it. Please update to version 4.1.3 of vscode-languageclient and see if it satisfies your need.

[dbaeumer]

@rcjsuen thanks for pointing @CRogers to this.

[vscodebot]

This issue has been closed automatically because it needs more information and has not had recent activity. See also our issue reporting guidelines.

Happy Coding!