Add Definition support for routes #331
Conversation
andyw8
force-pushed
the
andyw8/jumpt-to-route-definition
branch
from
db49e34
to
740a8a9
Compare
| @response_builder << Interface::Location.new( | ||
| uri: URI::Generic.from_path(path: file_path).to_s, | ||
| range: Interface::Range.new( | ||
| start: Interface::Position.new(line: Integer(line) - 1, character: 0), |
There was a problem hiding this comment.
in @tenderlove's prototype he gets the character position by looking for the first non-whitespace character on the line, but I'm not sure it's really necessary.
andyw8
force-pushed
the
andyw8/jumpt-to-route-definition
branch
from
740a8a9
to
ed1bf3c
Compare
|
Still a few things to finalize but this is ready for a first round of feedback. |
andyw8
force-pushed
the
andyw8/jumpt-to-route-definition
branch
from
ed1bf3c
to
6a97d0c
Compare
andyw8
force-pushed
the
andyw8/jumpt-to-route-definition
branch
from
6a97d0c
to
8cb5bcc
Compare
andyw8
force-pushed
the
andyw8/jumpt-to-route-definition
branch
from
8cb5bcc
to
dd36e2a
Compare
andyw8
force-pushed
the
andyw8/jumpt-to-route-definition
branch
2 times, most recently
from
75b69b5
to
23620ab
Compare
andyw8
force-pushed
the
andyw8/jumpt-to-route-definition
branch
3 times, most recently
from
2157fba
to
e92c3ed
Compare
andyw8
changed the title
andyw8
force-pushed
the
andyw8/jumpt-to-route-definition
branch
from
461e176
to
a85afe9
Compare
| # Older versions of Rails don't support `route_source_locations`. | ||
| # We also check it hasn't been disabled. | ||
| unless ActionDispatch::Routing::Mapper.respond_to?(:route_source_locations) && | ||
| ActionDispatch::Routing::Mapper.route_source_locations |
There was a problem hiding this comment.
Can this check's result be evaluated and stored on the addon-side so we just don't send requests for those older versions of Rails?
There was a problem hiding this comment.
🤔 So maybe instead of start returning just { message: "ok" }, it could also return a configuration hash?
There was a problem hiding this comment.
No I mean we can make the early return in handle_route instead?
We can also use version check instead of respond_to? if we want to avoid pulling Rails components there.
There was a problem hiding this comment.
We can't rely purely on the version check because the app could have an initializer which sets route_source_locations to false.
There was a problem hiding this comment.
I think for this first iteration I'd like to leave the approach as-is, and we can re-visit.
There was a problem hiding this comment.
If that's the case, then ActionDispatch::Routing::Mapper.route_source_locations needs to be in server.rb while ActionDispatch::Routing::Mapper.respond_to?(:route_source_locations) can be on the addon side. This is because only server.rb would be run with the application booted and initializers executed.
However, I also feel like it's not a user-facing option. Otherwise, it'd be accessible via config as well. From the search result, I feel that it's an internal option that's only controlled by Rails environment. If that's true, though, perhaps we don't even check its value as we'd always be running in dev environment anyway? 🤔
andyw8
force-pushed
the
andyw8/jumpt-to-route-definition
branch
3 times, most recently
from
cfc7c96
to
fb63730
Compare
| @@ -12,10 +12,8 @@ jobs: | |||
| gemfile: | |||
| - Gemfile | |||
| - gemfiles/Gemfile-rails-main | |||
| ruby: ["3.0", "3.1", "3.2", "3.3", "head"] | |||
| ruby: ["3.0", "3.1", "3.2", "3.3"] | |||
There was a problem hiding this comment.
It seems that the behaviour added in https://github.com/rails/rails/pull/47877/files is not working properly with Ruby head. Since Rails itself doesn’t test against Ruby head, I think it’s reasonable if we don’t either, and wait until this is fixed upstream.
andyw8
force-pushed
the
andyw8/jumpt-to-route-definition
branch
from
fb63730
to
e42d059
Compare
andyw8
force-pushed
the
andyw8/jumpt-to-route-definition
branch
from
e42d059
to
fca767d
Compare
| @@ -31,7 +31,7 @@ def activate(global_state, message_queue) | |||
| @global_state = T.let(global_state, T.nilable(RubyLsp::GlobalState)) | |||
| $stderr.puts("Activating Ruby LSP Rails addon v#{VERSION}") | |||
| # Start booting the real client in a background thread. Until this completes, the client will be a NullClient | |||
| Thread.new { @client = RunnerClient.create_client } | |||
| Thread.new { @client = RunnerClient.create_client }.join | |||
This PR adds support for the Definition request for Rails routes. This allow jumping from names route, e.g.
new_user_path, to the corresponding route declaration, e.g. inroutes.rb.The implementation is based on @tenderlove's prototype in https://github.com/tenderlove/refreshing, and uses the behaviour added by @luanzeba and others in rails/rails#47877.
Notes:
For Sorbet projects using the Tapioca DSL generators for routes, the Definition Lookup will return two results - one for the RBI file, and one for the actual route.
For Shopify folk: Unfortunately this doesn't work on Core, the source location always point to a
supercall inrouting_annotations.rb.bin/rails routes --expandedhas the same problem.TODO:
Testing
It's working well on Identity, (which doesn't use Sorbet) and Code DB (which does use Sorbet).