Closed spoenemann closed 9 months ago
After rethinking, I realized the workspace file operations shouldn't be used for triggering DocumentBuilder updates because they are called only before / after operations done by the user in the client (VS Code). I extracted those operations into a new service FileOperationHandler
.
I think issues such as #1230 need to be addressed with the file system watcher, which we can control from the language server after this change.
By the way: I tested the didChangeWatchedFiles
functionality in Gitpod with the arithmetics language:
I appended a non-correct string to the example file by executing echo "7*e;" >> example.calc
in a shell with having example.calc
both closed and opened, and error markers showed up immediately in both cases.
I'm just wondering whether Langium should bring a handler for didDeleteFiles notifications by default, as that might (should) have direct impact on the content of the index, doesn't it? (didCreateFiles is probably less critical as newly created files are probably empty initially)
Those events are covered by the file watcher. The only events we're currently missing are operations on the folder level (#1230).
You can add some tests for documentation purpose: to see how these services are used. As I understand, they are just handlers of LSP events.
I don't know what to test here. The default implementation doesn't do much. Regarding documentation, we should mention the new service in https://langium.org/docs/document-lifecycle/
@spoenemann
We should test this thoroughly, and also check what happens if the client still creates a FileSystemWatcher – does the language server receive the notifications twice? In that case, this is a breaking change that should be well documented in the CHANGELOG.
I tested this: The (VS Code) language client indeed sends two independent file change events - within a single notification -, see:
Wrt. to Langium's current implementation this seems to be fine, mainly because of https://github.com/eclipse-langium/langium/blob/d2cfea2effb0f24f7a5b9383d6935f68016f37ec/packages/langium/src/workspace/document-builder.ts#L167
However, I would prefer to add a deduplication in https://github.com/eclipse-langium/langium/blob/d2cfea2effb0f24f7a5b9383d6935f68016f37ec/packages/langium/src/lsp/document-update-handler.ts#L82-L84 (and document it's purpose clearly).
Btw. the same happens when moving files, all fs events are collected and sent via a single notification
However, I would prefer to add a deduplication
Why add a deduplication if the URIs are already deduplicated later? Deduplicating is expensive. I'd rather document this in the CHANGELOG so users don't configure the FS watcher on the client side.
My guess is that in a few weeks/month we'll have forgotten about that detail, and once the area in the document builder is somehow changed, potential issues pop up quickly.
Another option would be to have a test enforcing the tolerance of such duplicate URIs s.t. we make sure Langium can handle such cases properly.
I'd rather document this in the CHANGELOG so users don't configure the FS watcher on the client side.
I have existing adopters in mind, and testing the correct handling of file renamings is probably not considered after switching the Langium version.
Ok I used streams so the deduplication doesn't get too verbose.
Fixes #1272.
In addition, I stumbled over this statement from the LSP specification, didChangeWatchedFiles notification:
The default implementation of the new DocumentUpdateHandler service, which listens and reacts to
didChangeWatchedFiles
notifications, takes care of registering a file watcher so this doesn't have to be done by the client. This solves the problem I described in https://github.com/eclipse-langium/langium/issues/1230#issuecomment-1785356096:We should test this thoroughly, and also check what happens if the client still creates a FileSystemWatcher – does the language server receive the notifications twice? In that case, this is a breaking change that should be well documented in the CHANGELOG.