Switch to rails runner approach instead of Rack app
#256
Conversation
andyw8
force-pushed
the
andyw8/implement-lsp-runner
branch
from
e8b0135
to
35ff940
Compare
andyw8
force-pushed
the
andyw8/implement-lsp-runner
branch
3 times, most recently
from
41c27dc
to
c38937f
Compare
andyw8
force-pushed
the
andyw8/implement-lsp-runner
branch
3 times, most recently
from
cf92da8
to
5515f70
Compare
andyw8
force-pushed
the
andyw8/implement-lsp-runner
branch
from
5515f70
to
01047e0
Compare
andyw8
added
action-item
breaking-change
andyw8
changed the title
rails runner approach instead of Rack app
| module RubyLsp | ||
| module Rails | ||
| class Railtie < ::Rails::Railtie | ||
| config.ruby_lsp_rails = ActiveSupport::OrderedOptions.new |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- This addon was massively promoted by Chris Oliver recently
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Done (and verified against Code DB).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I suppose people may also have disabled this because they didn't like the extra noise in their logs.
andyw8
force-pushed
the
andyw8/implement-lsp-runner
branch
from
01047e0
to
77ae6c8
Compare
| @@ -1,11 +1,7 @@ | |||
| # typed: strict | |||
| # frozen_string_literal: true | |||
|
|
|||
| require "sorbet-runtime" | |||
There was a problem hiding this comment.
(sorbet-runtime is already required by ruby-lsp)
lib/ruby_lsp_rails/railtie.rb
Outdated
| initializer "ruby_lsp_rails.setup" do |_app| | ||
| config.after_initialize do |app| | ||
| config.ruby_lsp_rails.server | ||
| raise "not needed!" |
There was a problem hiding this comment.
Let's use a deprecation warning from Active Support instead. Otherwise, this will still break people's servers.
We can mention in the warning that the configuration can be removed.
There was a problem hiding this comment.
Yeah, was just was just testing with this, it's now a deprecation.
andyw8
force-pushed
the
andyw8/implement-lsp-runner
branch
3 times, most recently
from
f16624a
to
231cc6f
Compare
andyw8
force-pushed
the
andyw8/implement-lsp-runner
branch
from
231cc6f
to
2f3457d
Compare
andyw8
force-pushed
the
andyw8/implement-lsp-runner
branch
from
2f3457d
to
758ca85
Compare
This PR switches Ruby LSP to the new pipe approach rather than a mounted Rack server. (the runner was added already in #250).
This has several advantages, such as not needing the Rails server to be running. Previous discussion can be found in the prototype PR.
If an app was configured with
config.ruby_lsp_rails.server, this setting is no longer used and should be removed.Note that there is currently no automatic reloading, so a change such as a database migration will not be visible until Ruby LSP is restarted.