[Tapioca Add-on] Generate DSL RBIs in add-on mode #2093

KaanOzkan · 2024-11-26T16:43:03Z

Motivation

Extracting from main...tapioca-addon-feature-branch.

Implementation

Tests

Will be added later.

Co-authored-by: Alex Rocha <[email protected]> Co-authored-by: Andy Waite <[email protected]>

By leveraging file checksums using `Zlib.crc32`, we can avoid triggering the DSL generation process for files that have not changed.

KaanOzkan · 2024-11-26T19:13:31Z

lib/ruby_lsp/tapioca/server_addon.rb

@@ -13,13 +13,17 @@ def name
      def execute(request, params)
        case request
        when "dsl"
-          dsl(params)
+          fork do


We run the DSL generation in a short living process because it uses a lot of memory.

KaanOzkan · 2024-11-26T19:14:33Z

lib/ruby_lsp/tapioca/addon.rb

@@ -41,6 +46,7 @@ def activate(global_state, outgoing_queue)
        rescue IncompatibleApiError
          # The requested version for the Rails add-on no longer matches. We need to upgrade and fix the breaking
          # changes
+          puts "IncompatibleApiError: Cannot activate Tapioca LSP add-on"


I assume if we get this error @rails_runner_client will never be set and we'll return early in workspace_did_change_watched_files

We shouldn't puts in LSP related code. Although the Ruby LSP will override the default output device to prevent this from breaking communication, we should always standardize on window log messages.

Suggested change

puts "IncompatibleApiError: Cannot activate Tapioca LSP add-on"

outgoing_queue << Notification.window_log_message("IncompatibleApiError: Cannot activate Tapioca LSP add-on", type: Constant::MessageType::WARNING)

KaanOzkan · 2024-11-26T19:15:14Z

lib/tapioca/dsl/compiler.rb

+          @processable_constants = nil
+          @all_classes = nil
+          @all_modules = nil
+        end


These are memoized but in LSP mode we want them to be calculated every time.

andyw8 · 2024-11-26T20:02:00Z

lib/ruby_lsp/tapioca/addon.rb

@@ -63,7 +69,41 @@ def workspace_did_change_watched_files(changes)
        return unless T.must(@global_state).enabled_feature?(:tapiocaAddon)
        return unless @rails_runner_client # Client is not ready

-        nil
+        constants = changes.flat_map do |change|
+          path = URI(change[:uri]).to_standardized_path


(nit) Perhaps extract the body of this to its own method for readability?

I refactored the checksum logic into its own method. It's a bit longer and I don't love the boolean returns but I think main loop reads much better now. Lmk what you think.

I'm a little uncomfortable that file_updated? has side-effects. I'll take a look to see if there is another way we could structure it.

vinistock

Just two minor things

vinistock · 2024-11-26T20:11:50Z

lib/ruby_lsp/tapioca/addon.rb

@@ -41,6 +46,7 @@ def activate(global_state, outgoing_queue)
        rescue IncompatibleApiError
          # The requested version for the Rails add-on no longer matches. We need to upgrade and fix the breaking
          # changes
+          puts "IncompatibleApiError: Cannot activate Tapioca LSP add-on"


We shouldn't puts in LSP related code. Although the Ruby LSP will override the default output device to prevent this from breaking communication, we should always standardize on window log messages.

Suggested change

puts "IncompatibleApiError: Cannot activate Tapioca LSP add-on"

outgoing_queue << Notification.window_log_message("IncompatibleApiError: Cannot activate Tapioca LSP add-on", type: Constant::MessageType::WARNING)

lib/ruby_lsp/tapioca/addon.rb

andyw8 · 2024-11-27T15:32:42Z

lib/ruby_lsp/tapioca/addon.rb

+        when Constant::FileChangeType::DELETED
+          @file_checksums.delete(path)
+        else
+          T.must(@outgoing_queue) << Notification.window_log_message(


Are we anticipating other change types?

No, Sorbet was complaining and I thought it'd be nice to have an else here for the future.

alexcrocha · 2024-11-27T17:28:43Z

lib/ruby_lsp/tapioca/addon.rb

          @rails_runner_client.register_server_addon(File.expand_path("server_addon.rb", __dir__))
        rescue IncompatibleApiError
          # The requested version for the Rails add-on no longer matches. We need to upgrade and fix the breaking
          # changes
+          outgoing_queue << Notification.window_log_message(


Should this also be an instance variable?

Suggested change

outgoing_queue << Notification.window_log_message(

@outgoing_queue << Notification.window_log_message(

Maybe for consistency, but both are pointing to the same object.

andyw8 · 2024-11-27T17:37:24Z

lib/tapioca/commands/abstract_dsl.rb

@@ -77,6 +77,10 @@ def initialize(

      sig { params(outpath: Pathname, quiet: T::Boolean).returns(T::Set[Pathname]) }
      def generate_dsl_rbi_files(outpath, quiet:)
+        if @lsp_addon
+          pipeline.active_compilers.each(&:reset_state)


maybe extract this to pipeline#reset?

I think main issue is another method checking for lsp_addon. I don't want this code to be reusable for other clients. It's a unique thing (kind of hacky and not ideal 😬 ) for addon support.

KaanOzkan and others added 2 commits November 26, 2024 11:42

Generate DSL RBIs in add-on mode

977c8ba

Co-authored-by: Alex Rocha <[email protected]> Co-authored-by: Andy Waite <[email protected]>

Skip processing if the file hasn't changed

efa0e11

By leveraging file checksums using `Zlib.crc32`, we can avoid triggering the DSL generation process for files that have not changed.

KaanOzkan changed the title ~~Generate DSL RBIs in add-on mode~~ [Tapioca Add-on] Generate DSL RBIs in add-on mode Nov 26, 2024

KaanOzkan added the enhancement New feature or request label Nov 26, 2024

KaanOzkan requested review from andyw8, alexcrocha and vinistock November 26, 2024 18:32

KaanOzkan marked this pull request as ready for review November 26, 2024 18:32

KaanOzkan requested a review from a team as a code owner November 26, 2024 18:32

KaanOzkan commented Nov 26, 2024

View reviewed changes

andyw8 approved these changes Nov 26, 2024

View reviewed changes

andyw8 reviewed Nov 26, 2024

View reviewed changes

vinistock approved these changes Nov 26, 2024

View reviewed changes

KaanOzkan force-pushed the ko/tapioca-addon-3 branch from a4293e1 to 8610d6f Compare November 27, 2024 14:31

andyw8 reviewed Nov 27, 2024

View reviewed changes

alexcrocha reviewed Nov 27, 2024

View reviewed changes

andyw8 reviewed Nov 27, 2024

View reviewed changes

Refactor checksum & use outgoing_queue for logging

c630b69

KaanOzkan force-pushed the ko/tapioca-addon-3 branch from 8610d6f to c630b69 Compare November 27, 2024 18:42

KaanOzkan merged commit 3eb102a into main Nov 27, 2024
28 checks passed

KaanOzkan deleted the ko/tapioca-addon-3 branch November 27, 2024 19:22

shopify-shipit bot temporarily deployed to production November 27, 2024 22:03 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tapioca Add-on] Generate DSL RBIs in add-on mode #2093

[Tapioca Add-on] Generate DSL RBIs in add-on mode #2093

KaanOzkan commented Nov 26, 2024 •

edited

Loading

KaanOzkan Nov 26, 2024

KaanOzkan Nov 26, 2024 •

edited

Loading

vinistock Nov 26, 2024

KaanOzkan Nov 26, 2024

andyw8 Nov 26, 2024 •

edited

Loading

KaanOzkan Nov 26, 2024

andyw8 Nov 27, 2024

vinistock left a comment

vinistock Nov 26, 2024

andyw8 Nov 27, 2024

KaanOzkan Nov 27, 2024

alexcrocha Nov 27, 2024

andyw8 Nov 27, 2024

andyw8 Nov 27, 2024

KaanOzkan Nov 27, 2024

	puts "IncompatibleApiError: Cannot activate Tapioca LSP add-on"
	outgoing_queue << Notification.window_log_message("IncompatibleApiError: Cannot activate Tapioca LSP add-on", type: Constant::MessageType::WARNING)

	outgoing_queue << Notification.window_log_message(
	@outgoing_queue << Notification.window_log_message(

[Tapioca Add-on] Generate DSL RBIs in add-on mode #2093

[Tapioca Add-on] Generate DSL RBIs in add-on mode #2093

Conversation

KaanOzkan commented Nov 26, 2024 • edited Loading

Motivation

Implementation

Tests

Choose a reason for hiding this comment

KaanOzkan Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyw8 Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinistock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaanOzkan commented Nov 26, 2024 •

edited

Loading

KaanOzkan Nov 26, 2024 •

edited

Loading

andyw8 Nov 26, 2024 •

edited

Loading