Add pull diagnostic support #1534
Conversation
|
Am I correctly assuming that this would somewhat degrade the functionality or performance in editors that don't support pull diagnostics model since some code related to canceling validation requests is removed? I know this server is made by Microsoft employee and you are working for Microsoft too but it is used by many editors outside of VSCode and it would be nice to consider that. |
|
@rchl this is correct. However, keeping both implementations is not a good path forward either. I therefore suggest that we move the version number to 3.x. Then other editors can still use 2.x. |
server/src/eslintServer.ts
Outdated
| @@ -34,6 +34,7 @@ import LanguageDefaults from './languageDefaults'; | |||
|
|
|||
| // The connection to use. Code action requests get removed from the queue if | |||
| // canceled. | |||
| // TODO: Not sure if this changes with pull diagnostics. | |||
| @@ -57,7 +58,43 @@ const documents: TextDocuments<TextDocument> = new TextDocuments(TextDocument); | |||
| // So all validating will work out of the box since normal document events will fire. | |||
| const notebooks = new NotebookDocuments(documents); | |||
|
|
|||
| // This makes loading work in a plain NodeJS and a WebPacked environment | |||
| class DiagnosticVersions { | |||
There was a problem hiding this comment.
I am not sure if it is worth doing this. The unchanged support is for languages that have inter file dependencies. Since we don't handle these in ESLint right now (ESLint doesn't either) I think it is fair to awlays return the diagnostics since we re-validate a file when the user types. So diagnostic positions changeanyways.
There was a problem hiding this comment.
My reasoning behind doing this is because not all editors offer this kind of client configuration for indicating which events should trigger a pull. By having this we can ensure that diagnostics are updated based on the value of the eslint.run setting.
server/src/eslintServer.ts
Outdated
| async function validateSingle(document: TextDocument, publishDiagnostics: boolean = true): Promise<void> { | ||
| // We validate document in a queue but open / close documents directly. So we need to deal with the | ||
| async function validateSingle(params: DocumentDiagnosticParams): Promise<FullDocumentDiagnosticReport | UnchangedDocumentDiagnosticReport> { | ||
| // TODO: Not sure if this comment still applies. |
There was a problem hiding this comment.
This shouldn't happen anymore.
|
and thanks a lot for doing this. Highly appreciated. |
server/src/eslintServer.ts
Outdated
| if (refreshDiagnostics) { | ||
| connection.languages.diagnostics.refresh(); |
There was a problem hiding this comment.
Should not be called if client doesn't support pull.
Node libraries don't check client capabilities so this will trigger crash.
There was a problem hiding this comment.
Well, unless the intention is to completely stop supporting non-pull model...
Not sure if that's @dbaeumer meant in another comment.
There was a problem hiding this comment.
To agree with the LSP specs, I'll add the capability check before sending the request @rchl. Thanks for catching that!
|
My proposal is to only support pull model from 3.x on forward. It allows us to remove quite some code and makes the overall processing easier on the server side. |
This PR updates the server and client to use the pull diagnostics model.
Before completing this, there are a couple of places where I'm unsure if a change is needed (I marked those as
TODOs in the code):