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

Switch to rails runner approach instead of Rack app #256

Merged
merged 1 commit into from
Feb 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,15 @@ See the [documentation](https://shopify.github.io/ruby-lsp-rails) for more in-de
## How It Works

This gem consists of two components that enable enhanced Rails functionality in the editor:
When Ruby LSP Rails starts, it spawns a `rails runner` instance which runs
`[server.rb](https://github.com/Shopify/ruby-lsp-rails/blob/main/lib/ruby_lsp/ruby_lsp_rails/server.rb)`.
The addon communicates with this process over a pipe (i.e. `stdin` and `stdout`) to fetch runtime information about the application.

1. A Rack app that automatically exposes APIs when Rails server is running
1. A Ruby LSP addon that connects to the exposed APIs to fetch runtime information from the Rails server

This is why the Rails server needs to be running for some features to work.
When extension is stopped (e.g. by quitting the editor), the server instance is shut down.
andyw8 marked this conversation as resolved.
Show resolved Hide resolved

> [!NOTE]
> There is no need to restart the Ruby LSP every time the Rails server is booted.
> If the server is shut down, the extra features will temporarily disappear and reappear once the server is running again.
> Prior to v0.3, `ruby-lsp-rails` used a different approach which involved mounting a Rack application within the Rails app.
> That approach was brittle and susceptible to the application's configuration, such as routing and middleware.
## Contributing

Expand Down
3 changes: 0 additions & 3 deletions lib/ruby-lsp-rails.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
# typed: strict
# frozen_string_literal: true

require "sorbet-runtime"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(sorbet-runtime is already required by ruby-lsp)

require "pathname"

require "ruby_lsp_rails/version"
require "ruby_lsp_rails/railtie"

Expand Down
14 changes: 7 additions & 7 deletions lib/ruby_lsp/ruby_lsp_rails/addon.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

require "ruby_lsp/addon"

require_relative "rails_client"
require_relative "runner_client"
require_relative "hover"
require_relative "code_lens"

Expand All @@ -12,18 +12,18 @@ module Rails
class Addon < ::RubyLsp::Addon
extend T::Sig

sig { returns(RailsClient) }
sig { returns(RunnerClient) }
def client
@client ||= T.let(RailsClient.new, T.nilable(RailsClient))
@client ||= T.let(RunnerClient.new, T.nilable(RunnerClient))
end

sig { override.params(message_queue: Thread::Queue).void }
def activate(message_queue)
client.check_if_server_is_running!
end
def activate(message_queue); end

sig { override.void }
def deactivate; end
def deactivate
client.shutdown
end

# Creates a new CodeLens listener. This method is invoked on every CodeLens request
sig do
Expand Down
2 changes: 1 addition & 1 deletion lib/ruby_lsp/ruby_lsp_rails/hover.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class Hover

sig do
params(
client: RailsClient,
client: RunnerClient,
response_builder: ResponseBuilders::Hover,
nesting: T::Array[String],
index: RubyIndexer::Index,
Expand Down
77 changes: 0 additions & 77 deletions lib/ruby_lsp/ruby_lsp_rails/rails_client.rb

This file was deleted.

58 changes: 0 additions & 58 deletions lib/ruby_lsp_rails/rack_app.rb

This file was deleted.

25 changes: 4 additions & 21 deletions lib/ruby_lsp_rails/railtie.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,34 +2,17 @@
# frozen_string_literal: true

require "rails"
require "ruby_lsp_rails/rack_app"

module RubyLsp
module Rails
class Railtie < ::Rails::Railtie
config.ruby_lsp_rails = ActiveSupport::OrderedOptions.new
Copy link
Member

Choose a reason for hiding this comment

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

Let's maybe keep the railtie so we can print deprecation warning for apps assigning config.ruby_lsp_rails.server?

I think upgrading ruby-lsp-rails and having the app blown up is not a good impression we want users to have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that originally, but after discussing with @vinistock, probably very few apps are likely to be using this.

For Core, we can remove the setting when updating the gem.

We're also tagging this PR as a 'breaking change' for awareness.

Copy link
Member

Choose a reason for hiding this comment

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

We're also tagging this PR as a 'breaking change' for awareness.

Yes, this and the version change will technically justify it, but they won't show up in the users' bump PR I think.

Given that:

  1. This addon was massively promoted by Chris Oliver recently
  2. Most Rails apps are private and won't show up in the GH search result

I don't want to underestimate the negative effect this may cause to our users, especially when it's actually not too costly to avoid.

Copy link
Member

Choose a reason for hiding this comment

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

I highly doubt a lot of people are turning off the server, but if you feel strongly about this we can add a deprecation warning and then remove it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (and verified against Code DB).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks 👍

I want to be super careful about this because I did already find 2 (though inactive) repos having this set just doing a quick search.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose people may also have disabled this because they didn't like the extra noise in their logs.

config.ruby_lsp_rails.server = true

initializer "ruby_lsp_rails.setup" do |_app|
config.after_initialize do |app|
# If we start the app with `bin/rails console` then `Rails::Server` is not defined.
if defined?(::Rails::Server) && config.ruby_lsp_rails.server
app.routes.prepend do
T.bind(self, ActionDispatch::Routing::Mapper)
mount(RackApp.new => RackApp::BASE_PATH)
end

ssl_enable, host, port = ::Rails::Server::Options.new.parse!(ARGV).values_at(:SSLEnable, :Host, :Port)
app_uri = "#{ssl_enable ? "https" : "http"}://#{host}:#{port}"
app_uri_path = ::Rails.root.join("tmp", "app_uri.txt")
app_uri_path.write(app_uri)

at_exit do
# The app_uri.txt file should only exist when the server is running. The addon uses its presence to
# report if the server is running or not. If the server is not running, some of the addon features
# will not be available.
File.delete(app_uri_path) if File.exist?(app_uri_path)
end
config.after_initialize do |_app|
unless config.ruby_lsp_rails.server.nil?
ActiveSupport::Deprecation.new.warn("The `ruby_lsp_rails.server` configuration option is no longer " \
"needed and will be removed in a future release.")
end
end
end
Expand Down
13 changes: 0 additions & 13 deletions test/ruby_lsp_rails/addon_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,6 @@ class AddonTest < ActiveSupport::TestCase
addon = Addon.new
assert_equal("Ruby LSP Rails", addon.name)
end

test "activate checks if Rails server is running" do
rails_client = stub("rails_client", check_if_server_is_running!: true)

RubyLsp::Rails::RailsClient.stubs(instance: rails_client)

addon = Addon.new
queue = Thread::Queue.new

capture_io { assert(addon.activate(queue)) }
ensure
T.must(queue).close
end
end
end
end
30 changes: 14 additions & 16 deletions test/ruby_lsp_rails/hover_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ module RubyLsp
module Rails
class HoverTest < ActiveSupport::TestCase
setup do
File.write("#{Dir.pwd}/test/dummy/tmp/app_uri.txt", "http://localhost:3000")
@client = RailsClient.new
@message_queue = Thread::Queue.new

# Build the Rails documents index ahead of time
Expand All @@ -23,7 +21,7 @@ class HoverTest < ActiveSupport::TestCase

test "hook returns model column information" do
expected_response = {
schema_file: "#{@client.root}/db/schema.rb",
schema_file: "#{dummy_root}/db/schema.rb",
columns: [
["id", "integer"],
["first_name", "string"],
Expand All @@ -34,8 +32,7 @@ class HoverTest < ActiveSupport::TestCase
],
}

stub_http_request("200", expected_response.to_json)
@client.stubs(:check_if_server_is_running!)
RunnerClient.any_instance.stubs(model: expected_response)

response = hover_on_source(<<~RUBY, { line: 3, character: 0 })
class User < ApplicationRecord
Expand All @@ -50,7 +47,7 @@ class User < ApplicationRecord
```
**Definitions**: [fake.rb](file:///fake.rb#L1,1-2,4)
[Schema](file://#{@client.root}/db/schema.rb)
[Schema](file://#{dummy_root}/db/schema.rb)
**id**: integer
Expand All @@ -69,7 +66,7 @@ class User < ApplicationRecord

test "return column information for namespaced models" do
expected_response = {
schema_file: "#{@client.root}/db/schema.rb",
schema_file: "#{dummy_root}/db/schema.rb",
columns: [
["id", "integer"],
["first_name", "string"],
Expand All @@ -80,8 +77,7 @@ class User < ApplicationRecord
],
}

stub_http_request("200", expected_response.to_json)
@client.stubs(:check_if_server_is_running!)
RunnerClient.any_instance.stubs(model: expected_response)

response = hover_on_source(<<~RUBY, { line: 4, character: 6 })
module Blog
Expand All @@ -93,7 +89,7 @@ class User < ApplicationRecord
RUBY

assert_equal(<<~CONTENT.chomp, response.contents.value)
[Schema](file://#{@client.root}/db/schema.rb)
[Schema](file://#{dummy_root}/db/schema.rb)
**id**: integer
Expand All @@ -111,12 +107,11 @@ class User < ApplicationRecord

test "handles `db/structure.sql` instead of `db/schema.rb`" do
expected_response = {
schema_file: "#{@client.root}/db/structure.sql",
schema_file: "#{dummy_root}/db/structure.sql",
columns: [],
}

stub_http_request("200", expected_response.to_json)
@client.stubs(:check_if_server_is_running!)
RunnerClient.any_instance.stubs(model: expected_response)

response = hover_on_source(<<~RUBY, { line: 3, character: 0 })
class User < ApplicationRecord
Expand All @@ -127,7 +122,7 @@ class User < ApplicationRecord

assert_includes(
response.contents.value,
"[Schema](file://#{@client.root}/db/structure.sql)",
"[Schema](file://#{dummy_root}/db/structure.sql)",
)
end

Expand All @@ -137,8 +132,7 @@ class User < ApplicationRecord
columns: [],
}

stub_http_request("200", expected_response.to_json)
@client.stubs(:check_if_server_is_running!)
RunnerClient.any_instance.stubs(model: expected_response)

response = hover_on_source(<<~RUBY, { line: 3, character: 0 })
class User < ApplicationRecord
Expand Down Expand Up @@ -213,6 +207,10 @@ def hover_on_source(source, position)
assert_nil(response.error)
response.response
end

def dummy_root
File.expand_path("#{__dir__}/../../test/dummy")
end
end
end
end
Loading
Loading