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

Prototype LSP Rails runner #230

Closed
wants to merge 6 commits into from
Closed

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Jan 14, 2024

Currently with ruby-lsp-rails, the Rails server needs to be running so that the LSP can make requests to it, since requests are sent over HTTP.

This PR demonstrate a different approach we are considering. The LSP gem will spawn a separate Rails instance, and we will use stdin/stdout for communication. For prototyping purposes, we are using rails runner, but for a real implementation this could be a new Rails command, such as rails lsp.

The LSP gem will also be responsible for shutting down the server. (The example here currently implements that command only.)

The protocol will be similar to that of the LSP protocol – content-length header, followed by two newlines, then a JSON structure.

Since we control both sides of the protocol, we can change it as needed, so we don't need to be too concerned about the structure.

We're looking for general feedback on:

  • Does this approach make sense overall?
  • Is there any functionality that would become impossible without being embedded in the Rails server?
  • Are there any other gotchas we need to be aware of early on?

@andyw8 andyw8 force-pushed the andyw8/prototype-lsp-runner branch from 7bf3710 to a14bebe Compare January 14, 2024 23:23
lib/ruby_lsp/ruby_lsp_rails/addon.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/ruby_lsp_rails/addon.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/ruby_lsp_rails/addon.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/ruby_lsp_rails/server.rb Outdated Show resolved Hide resolved
@andyw8 andyw8 changed the title Prototype LSP runner Prototype LSP runner approach Jan 15, 2024
@andyw8 andyw8 changed the title Prototype LSP runner approach Prototype LSP Rails runner Jan 15, 2024
@andyw8
Copy link
Contributor Author

andyw8 commented Jan 15, 2024

👋 @casperisfine @tenderlove we would appreciate any feedback you have on this.

Copy link

@casperisfine casperisfine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this approach make sense overall?

IMO yes.

Is there any functionality that would become impossible without being embedded in the Rails server?

Not that I can think of? The only thing is people would manually restart their server when doing a change in a non-reloaded place. Here it's unclear how it should work, but there are ways to detect that with inotify or something. But not strictly required for MVP IMO.

Are there any other gotchas we need to be aware of early on?

I can't think of any. You may need to complexify your thing a bit to deal with autoloading / reloading, as you'll likely want to inspect autoloaded constants etc.

Reloading everything on every requests might be slow, but it can be optimized later (also with FS events)

lib/ruby_lsp/ruby_lsp_rails/addon.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/ruby_lsp_rails/addon.rb Outdated Show resolved Hide resolved
andyw8 and others added 2 commits January 16, 2024 14:08
@tenderlove
Copy link

  • Does this approach make sense overall?

It does make sense, and it's initially the thing I was thinking of doing. However, after talking with @paracycle extensively about the ideas, we'd come up with a different way to do the integration.

  1. Extend the Info Controller to return introspection data as JSON.
  2. Add a route to the info controller such that the language server could register a callback URL

(The InfoController routes are registered here

When I say "introspection data" I mean being able to do something like:

GET /rails/info/model.json?name=User would return a JSON blob with information about the User model. The language server could make the request then parse the JSON. I'm not sure exactly what routes we'd need to add, I think it depends on the information we want to bubble up to the editor.

For the second point, I'd like it if when there's a runtime error (say someone loads a page in development and it 500's) that runtime error could be communicated back to the language server. In order to do that I was thinking the language server could do something like:

POST /rails/info/callback?url=http://localhost:123456/callback. Rails could keep track of the callback URL (in this case http://localhost:123456/callback), and when an exception occurs (maybe we could even make this work with test failures) it could post back to that URL.

The upsides of this overall approach are:

  1. It's language server agnostic, Rails doesn't need to know what a language server is.
  2. We don't have to teach the Rails server process how to speak any other protocol besides HTTP.
  3. Easy to extend (just add more routes / info to the InfoController
  4. Easy to test: we already have tests for the InfoController, we just add more and assert the JSON responses

Downsides that I've thought of (so far):

  1. Some added complexity with regard to tracking the callback URL and pushing data to the language server
  2. Can't be upgraded independently of Rails (the InfoController is part of Rails so we'd have to update Rails)
  3. Must go through all middleware

Point number 1 seems unavoidable regardless of the protocol. I think we could address point 2 and 3 by making a gem that provides a middleware. The main issue with a separate gem is that I'd expect we want to use internal APIs in order to provide introspection data. We'd have to figure out a way to prevent the gem from breaking when Rails internals change (this isn't impossible to do, it's just more work).

@casperisfine
Copy link

  1. Extend the Info Controller to return introspection data as JSON.

The main reason I suggested not to make this a controller is because you then need rails server running for the integration to even work, I don't think it's good UX. The language server should be able to automatically spawn the necessary process as a daemon in my opinion.

Using a dedicated process also entirely sidestep the issues with middlewares etc you mention.

All the callback protocol you mention is still possible with an independent process.

@andyw8
Copy link
Contributor Author

andyw8 commented Jan 18, 2024

Thank you both for the valuable feedback so far.

I’ll address some of the points raised, but @vinistock suggested a discussion may help to get us aligned. I’ve sent out an invite for that (with Ufuk and Rafael as optional attendees). So feel free to either respond here, or hold off until the call.

 I'd like it if when there's a runtime error (say someone loads a page in development and it 500's) that runtime error could be communicated back to the language server.

We’re unclear on what the benefit of this is, since the person would presumably already have the page open and could see the error, and would need to reload it in the browser after the error was fixed. Did you have a particular use case in mind?

  • It's language server agnostic, Rails doesn't need to know what a language server is.
  • We don't have to teach the Rails server process how to speak any other protocol besides HTTP.

Wouldn't both of these also be true with the 'runner' approach? The 'protocol' would be in the gem, rather than Rails itself.

Rails could keep track of the callback URL, and when an exception occurs it could post back to that URL.

We’re unclear on how this would work, since the LSP doesn’t currently expose an HTTP server. So we would need to have an additional HTTP server running. We’re wondering if it justifies the additional complexity.

@vinistock
Copy link
Member

Interesting question raised by Alex: is there any possibility that spawning the lightweight server through Rails runner could conflict in any way with the Rails server spawned by the developer? For example, some file lock?

@casperisfine
Copy link

conflict in any way with the Rails server spawned by the developer? For example, some file lock?

Not that I think of. The overwhelming majority of locks are in memory mutexes. No of course you are loading user code (the application itself), so anything could happen in it.

But generally speaking, loading rails s and rails c concurrently works. If not it's a bug, and so it's the same for rails r.

@andyw8
Copy link
Contributor Author

andyw8 commented Feb 14, 2024

#256

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.

4 participants