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

Add support for server addons #454

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Sep 18, 2024

Co-authored with @vinistock.

This is initially to support the Tapioca LSP.

There is some tricky logic involving two mutexes that was needed to ensure things are loaded in the correct order.

@andyw8 andyw8 added the enhancement New feature or request label Sep 18, 2024
@andyw8 andyw8 force-pushed the andyw8/vinistock/server-architecture-exploration branch from 1ebb9c0 to d749d1b Compare September 18, 2024 18:13
@andyw8 andyw8 marked this pull request as ready for review September 18, 2024 18:17
@andyw8 andyw8 requested a review from a team as a code owner September 18, 2024 18:17
@andyw8
Copy link
Contributor Author

andyw8 commented Sep 18, 2024

FYI @KaanOzkan

@andyw8 andyw8 marked this pull request as draft September 18, 2024 18:32
@@ -96,6 +96,14 @@ def initialize
raise InitializationError, @stderr.read
end

sig { params(server_addon_path: String).void }
def register_server_addon(server_addon_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need both inherited and register_server_addon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 perhaps we don't.... @vinistock ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I wanted to bring up for context that we need a way to limit who can use our addon. Right now we can do it in the addon while triggering register_server_addon.

Copy link
Member

Choose a reason for hiding this comment

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

They run on different Ruby processes, so I'm not sure how we could combine theses two.

Also, why do we need to limit who uses the addon?

Copy link
Contributor

@KaanOzkan KaanOzkan Sep 20, 2024

Choose a reason for hiding this comment

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

I thought it wasn't necessary but now I see that inherited is only triggered through the manual require we do in the server so we also need register_server_addon.

Also, why do we need to limit who uses the addon?

We need to do some testing first on a partial rollout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to do some testing first on a partial rollout.

We can manage that on the Tapioca side though, right?

Copy link
Member

Choose a reason for hiding this comment

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

Currently, there's no way to perform a partial rollout. But we could inform addons if experimental features are enabled so that they can decide whether to do something or not.

That would give you some control.

Copy link
Contributor

Choose a reason for hiding this comment

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

As I said above, with this implementation, rollout can be controlled by the individual addons through the trigger of register_server_addon. It's not an issue.

lib/ruby_lsp/ruby_lsp_rails/addon.rb Show resolved Hide resolved
@andyw8 andyw8 marked this pull request as ready for review September 20, 2024 14:56
@andyw8 andyw8 requested a review from st0012 September 20, 2024 14:57
@andyw8 andyw8 mentioned this pull request Sep 20, 2024
lib/ruby_lsp/ruby_lsp_rails/addon.rb Show resolved Hide resolved
@@ -96,6 +96,14 @@ def initialize
raise InitializationError, @stderr.read
end

sig { params(server_addon_path: String).void }
def register_server_addon(server_addon_path)
Copy link
Member

Choose a reason for hiding this comment

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

They run on different Ruby processes, so I'm not sure how we could combine theses two.

Also, why do we need to limit who uses the addon?

lib/ruby_lsp/ruby_lsp_rails/server.rb Outdated Show resolved Hide resolved
@andyw8 andyw8 force-pushed the andyw8/vinistock/server-architecture-exploration branch from 462564c to a248a32 Compare September 20, 2024 18:35
@andyw8 andyw8 enabled auto-merge (squash) September 20, 2024 18:35
@andyw8 andyw8 merged commit d39ed11 into main Sep 20, 2024
39 of 40 checks passed
@andyw8 andyw8 deleted the andyw8/vinistock/server-architecture-exploration branch September 20, 2024 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants