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

Avoid generic type in Netty's RunAsync #3529

Merged
merged 1 commit into from
Feb 21, 2024
Merged

Conversation

fwbrasil
Copy link
Contributor

Problem

The apply method in RunAsync takes an unnecessary type parameter. Kyo allows nesting of effects for composition but it's necessary to prove that a computation isn't nested to invoke methods that handle effects, which is necessary for RunAsync's implementation. I'm currently using an unsafe flat check to work this around.

Solution

Modify RunAsync.apply to take a an F fixed to Unit, which Kyo is able to prove as flat and matches the usage in NettyBodyListener.

Notes

  • I imagine this isn't considered a breaking change since the trait is internal.
  • I'd like to keep the integration with Tapir in Kyo's repository, which is why I'm using internal APIs. Is there an interest in providing first-class support for external integrations?
  • I had trouble to run the tests locally, I see odd errors like Referring to non-existent class sttp.apispec.SchemaType but I imagine isn't related to the change.

@adamw adamw merged commit bee7fd7 into softwaremill:master Feb 21, 2024
23 checks passed
@adamw
Copy link
Member

adamw commented Feb 21, 2024

Thanks!

As for your notes:

  • yes, that's fine, only the core module has strict compatibility guarantees
  • I think kyo is at a point where it evolves faster than tapir, so keeping the integration in kyo's repository would make most sense
  • hm not sure I saw this one, but sometimes if you try building for scala 2, you need to rebuild core (using core/clean from sbt). Some macro-related problems which I never deeply investigated. Works fine if you run tests in the Scala 3 modules.

@fwbrasil
Copy link
Contributor Author

Thanks for the quick review! Good point regarding the evolution pace. I worry about long-term maintenance, though. Is first-class support for external integrations without requiring private APIs something you'd be interested in? It seems it wouldn't be too hard to refactor things to simplify the extension points.

@fwbrasil fwbrasil deleted the runasync branch February 21, 2024 21:05
@adamw
Copy link
Member

adamw commented Feb 22, 2024

@fwbrasil do you mean adding Kyo integration to the tapir repository? If yes, then it's of course possible - we already have quite a lot of such integrations (see https://github.com/softwaremill/tapir/tree/master/integrations, and the various server interpreters)

Or did you mean something else?

@fwbrasil
Copy link
Contributor Author

No, I'd like to keep the integration in Kyo's repository but relying on internal APIs for that can become problematic in the long run.

@adamw
Copy link
Member

adamw commented Feb 22, 2024

@fwbrasil Ah I see - yes definitely - we should enable integration without having to use internal things :)

@adamw
Copy link
Member

adamw commented Feb 22, 2024

(and anything internal should be properly scoped, anyway)

@fwbrasil
Copy link
Contributor Author

Great! Thank you for the flexibility. I'm working on finalizing the integration and will add an item to my backlog to propose a way to avoid internal APIs. My focus is netty so it might be specific to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants