Introduce DocumentUpdateHandler service

[spoenemann]

Fixes #1272.

In addition, I stumbled over this statement from the LSP specification, didChangeWatchedFiles notification:

It is recommended that servers register for these file system events using the registration mechanism. In former implementations clients pushed file events without the server actively asking for it.

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 #1230 (comment):

...what I don't like about it is that we have a distribution of responsibilities between language client and language server code. It would be better to find a solution where watched file system events are specified fully on the client or server side.

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.

[spoenemann]

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.

[sailingKieler]

Makes I sense for me.
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)

[sailingKieler]

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.

[spoenemann]

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).

[Lotes]

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.

[spoenemann]

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/

[sailingKieler]

@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

langium/packages/langium/src/workspace/document-builder.ts

Line 167 in d2cfea2

const allChangedUris = stream(changed).concat(deleted).map(uri => uri.toString()).toSet();

However, I would prefer to add a deduplication in

langium/packages/langium/src/lsp/document-update-handler.ts

Lines 82 to 84 in d2cfea2

	const changedUris = params.changes.filter(c => c.type !== FileChangeType.Deleted).map(c => URI.parse(c.uri));
	const deletedUris = params.changes.filter(c => c.type === FileChangeType.Deleted).map(c => URI.parse(c.uri));
	this.fireDocumentUpdate(changedUris, deletedUris);

(and document it's purpose clearly).

[sailingKieler]

Btw. the same happens when moving files, all fs events are collected and sent via a single notification

[spoenemann]

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.

[sailingKieler]

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.

[sailingKieler]

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.

[spoenemann]

Ok I used streams so the deduplication doesn't get too verbose.

[sailingKieler]

👍