Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

didChangeWatchedFiles using watchman #1674

Closed
wants to merge 2 commits into from
Closed

didChangeWatchedFiles using watchman #1674

wants to merge 2 commits into from

Conversation

tomv564
Copy link
Contributor

@tomv564 tomv564 commented May 22, 2021

This is a proposal for an (interim?) solution for users who want file change notifications to their servers using facebook watchman before a potential API for this arrives in Sublime (so, issue #892)

I know Raoul commented that having a "hard" dependency (or a python package or a binary) would introduce a fair bit of maintenance pain - my thought would be to enable the support if the user opts-in and has watchman on their path for that reason.
It uses a named socket, so it is otherwise similar to TCP communication to language servers.

This was only partially tested using Metals which has a pretty nice set of globs it requests.
It would need quite a bit more testing:

  • Windows
  • A bunch of servers (javascript/typescript servers are probably important, any other likely heavy users?)
  • Ensure watchers and watchman are correctly being removed when sessions, LSP or sublime are shut down.

@rchl
Copy link
Member

rchl commented May 22, 2021

Haven't had a proper look at your changes yet but would just say that I have a "competing" solution that I've been using privately and is based on chokidar node package. It's not really finished though and there is likely no agreement whether we'd want that, even if finished.

Also, you seem to be reimplementing glob handling which we should already have support for through the wcmatch dependency

@tomv564
Copy link
Contributor Author

tomv564 commented May 22, 2021

Hi @rchl! Saw your work also, but I may have misunderstood that you wanted to solve it through a few LSP-* language packages only?

Thanks for the hint about wcmatch - there is no wish to implement glob matching on the LSP side, but watchman did not understand the {alternative1, alternativ2} syntax. That might also make the watchman approach pointless, if servers are many glob features that watchman doesn't understand: https://facebook.github.io/watchman/docs/expr/match.html

@rwols
Copy link
Member

rwols commented May 22, 2021

The glob patterns are described here: https://microsoft.github.io/language-server-protocol/specification.html#documentFilter

If watchman understands a subset we would need some kind of translation layer

@rchl
Copy link
Member

rchl commented May 22, 2021

Hello :)

You are right. I forgot that it's not LSP that has to support it but the watcher itself...

Using chokidar at least guarantees that the LSP spec requirements are supported. For example vetur (my main usecase for this feature) uses a {**/*.js,**/*.ts,**/*.json} pattern.

It's true that currently my solution only works within the LSP-* packages that use lsp_utils. We could bring it into the core but the problem is that we don't really agree on whether we want it since it in theory gonna be a resource hog (although at least from my experience with LSP-vue and LSP-typescript, it isn't).

@tomv564
Copy link
Contributor Author

tomv564 commented May 23, 2021

I agree the chokidar solution makes sense for the (majority?) of users who are already using node-based servers as it's likely much more complete.

What would it take for your solution to become something like a LSP-watcher-chokidar package that works for all language servers?
Can I support in some way to make that happen?

Could we leave LSP interface watcher so that I could contribute an LSP-watcher-watchman package in the future?

@rchl
Copy link
Member

rchl commented May 24, 2021

We should first agree on what everyone wants so that we don't work on something that won't be accepted later.

Do we want to extend LSP with some interface to allow packages like LSP-watcher-* to enable the watching functionality for LSP?

That would require supporting registering capability dynamically and also supporting the watcher configuration in the client configuration since some servers (LSP-vue, LSP-eslint) don't use dynamic capabilities for that and require client to define the watchers.

@rwols
Copy link
Member

rwols commented May 24, 2021

What would it take for your solution to become something like a LSP-watcher-chokidar package that works for all language servers?

I think that could go in lsp_utils, right?

Could we leave LSP interface watcher so that I could contribute an LSP-watcher-watchman package in the future?

There are these two callbacks for plugins:

    def on_register_capability_async(self, registration_id: str, capability_path: str, options: Dict[str, Any]) -> None:
        """
        Notifies about server dynamically registering a capability using the client/registerCapability request.
        This API is triggered on async thread.

        :param registration_id: The registration identifier
        :param capability_path: The registration capability path
        :param options: The registration options
        """
        pass

    def on_unregister_capability_async(
        self, registration_id: str, capability_path: str, options: Dict[str, Any]
    ) -> None:
        """
        Notifies about server un-registering a capability using the client/unregisterCapability request.
        This API is triggered on async thread.

        :param registration_id: The registration identifier
        :param capability_path: The registration capability path
        :param options: The registration options
        """
        pass

@rchl
Copy link
Member

rchl commented May 24, 2021

What would it take for your solution to become something like a LSP-watcher-chokidar package that works for all language servers?

I think that could go in lsp_utils, right?

It could but would we then make LSP depend on lsp_utils? Because how else would user enable the watching functionality? It's not possible to install a dependency on its own and installing some other server to enable that functionality would be "weird".

There are these two callbacks for plugins:

I think we want to support that functionality on the ClientConfig level, even without having a plugin.

@tomv564
Copy link
Contributor Author

tomv564 commented May 26, 2021

For simplicity's sake I suggest a register_watcher API that has nothing to do with ClientConfig or the existing plugin setup - the logic should not be server-specific I think?

some servers (LSP-vue, LSP-eslint) don't use dynamic capabilities for that

This is not protocol based, right? Would it suffice to expose a request_watch API for these plugins to use?

@rchl
Copy link
Member

rchl commented May 26, 2021

But to use the API one needs to create an LSP-* plugin right? In that case I already have solution that does that. But that means that clients defined within the LSP.sublime-settings can't use that functionality.

It might be OK of course but I wanted to have a complete solution that we could just plug-in the ST API into in the future, once its ready.

This is not protocol based, right? Would it suffice to expose a request_watch API for these plugins to use?

The notifications (didChangeWatchedFiles) are of course dispatched through the protocol. The registration of the watcher is completely outside of the server knowledge.

@rchl
Copy link
Member

rchl commented Jun 26, 2021

What would it take for your solution to become something like a LSP-watcher-chokidar package that works for all language servers?

Created something like that at #1764

@rwols
Copy link
Member

rwols commented Aug 7, 2021

superseded by #1764

@rwols rwols closed this Aug 7, 2021
@rchl
Copy link
Member

rchl commented Oct 4, 2021

Now also released implementation through an additional package https://packagecontrol.io/packages/LSP-file-watcher-chokidar
See its readme for more info.

@rchl rchl deleted the feature-watchman branch November 6, 2021 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants