Modifying `structure.sql` doesn't trigger an update event

[Earlopain]

Reloading for schema.rb and structure.sql was added in #282. The server however only listens for *.rb changes so it will never reload for strucutre.sql changes.

Culprit in the server code: https://github.com/Shopify/ruby-lsp/blob/729df3e909368ee09ba1315fa0a208dda52110ea/lib/ruby_lsp/server.rb#L206-L212

Demo video:

Screencast_20240409_154242.webm

[vinistock]

Thank you for the bug report! Yeah, we need to watch the db/structure.sql files from the base Ruby LSP to fix this.

[Earlopain]

What do you think about a possibility for addons to register additional file events instead? It is more complicated though

[vinistock]

That's a great idea, there may be other cases where it makes sense for an addon to watch a file that isn't Ruby (like some configuration file).

Addons already have the ability to send messages back to the editor, the only piece missing is that we have to first check if the editor connecting to the server supports both file watching and dynamic feature registrations (which we do here).

I think we can put the information about whether the editor supports those or not in the GlobalState, which is already available to addons. That way, Ruby LSP Rails can check if file watching is possible and send the message registering for db/*_structure.sql files itself.

[Earlopain]

Yes, that sounds fine I think. I will start with the change in ruby-lsp. I see now how addons would send the request, just throw it in the message_queue.

[Earlopain]

I have a question for this, I want to do something like the following:

message_queue << Request.new(
  id: "ruby-lsp-rails-register-watched-files",
  method: "client/registerCapability",
  params: Interface::RegistrationParams.new(
    registrations: [],
  ),
)

This doesn't work because the ids are typed as integers only and are accessible in the server exclusivly as far as I can tell. Is there a way I'm missing to send a request from an addon without grabing a high enough number and praying it doesn't collide?

Or perhaps a different question, do these ids have to be numerically incrementing or is a unique identifier enough? I'm not very familiar with the design of LSPs

[vinistock]

This is a really good point. The spec says that ID can be a string too, but I have no idea if it'll accept any arbitrary string.

However, this does seem like a small flaw in our design. It might be better to not pass id in the Request initializer (or make it optional) and merge the @current_request_id when we are writing messages back to the editor - if the message is a Request and not a Notification or Response.

[Earlopain]

I think I understand it now, the ID is just what will be set in the response in order to connect request/response pairs (see spec docs of Reponse just below). In that case, transparently setting the id isn't really helpful if someone wants to react to a certain response. OTOH, I'm not sure if you even have a chance to get at that response if you wanted to in an addon

I think it makes most sense to just accept strings as well. Addons should maintain their own counter and namespace their requests, if they even need a counter. For this particular issue a manual id like in my example is good since only one request will be sent anyways and I don't care about the response.

What do you think?

[vinistock]

Indeed, there's no way for an addon to receive a response back for their request yet. Maybe we can use a pattern as part of the ID, so that if one day we look into delegating the responses back to addons, it's easy to figure it out. For example, we could use the addon name.

For the time being, yeah, let's move forward with some generic string ID, like "request-file-watching".