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

Future[T] and Promise[T] as Controller ReturnTypes #51

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

domdorn
Copy link

@domdorn domdorn commented May 9, 2024

This PR adds support for Future[T] and Promise[T] as return types in resources, e.g.

@Path("/")
class GreetingResource() 
    @GET
    @Path("/greet/promise")
    def promiseGreeting(): Promise[String] =
      Promise.successful("Hello from the promise")
    end promiseGreeting
end GreetingResource

I also added tests in the integration module and converted them to be in Scala

@domdorn
Copy link
Author

domdorn commented May 9, 2024

I've reworked this PR so that everything that resolves around using Future[T] or Promise[T] is in it own sub-extension. In another PR I'll add support for ZIO[R,E,A] in the same manner.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Thanks for the PRs, it's nice to see some activity in this project.

I added some suggestions, some of them minor, some of them not so much :).

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@gsmet
Copy link
Member

gsmet commented May 10, 2024

BTW, I suggest we get this one in shape and merged, before applying the same changes to the ZIO PR.

domdorn and others added 3 commits May 10, 2024 17:52
Co-authored-by: Guillaume Smet <[email protected]>
Co-authored-by: Guillaume Smet <[email protected]>
Co-authored-by: Guillaume Smet <[email protected]>
@gsmet
Copy link
Member

gsmet commented May 10, 2024

Disclaimer: I have no knowledge of Scala, so please don't hesitate to push back if what I say doesn't make sense :).

BTW, I think you could put everything in the same module.

The build item machinery should be clever enough to not trigger the loading of the classes you added if the Quarkus REST extension is not around.
My guess is that you could simplify the integration by having the SPI dependencies around but quarkus-rest would be an optional dependency.

I think that would have my preference if you can make it work as the experience will be better for the users.

@domdorn
Copy link
Author

domdorn commented May 10, 2024

@gsmet I've updated the PR, please have a look. I noticed that I don't get the stack trace in the native executable in case of an error, so I removed the check on the body (in the tests).
I had everything in the same extension but when adding the zio one I figured it would be cleaner to have it in its own extension, especially if one day we'll add other effect types (cats/monix/whatever) as well..

@carlosedp
Copy link

Hey folks, any news on this?

@domdorn
Copy link
Author

domdorn commented Jun 1, 2024

I think I've applied everything that was requested, but maybe I missed something.
Anyways, I'm going to be on holidays the next 3 weeks without spare time to work on this.
From my point of view, the potentially remaining issues are just about moving code to the right places.
If anyone wants to fork/take over the PR, please do so!

@carlosedp
Copy link

@domdorn I think it's pretty good! We just need to wait review from @gsmet ... he might be pretty busy with recent Quarkus release.

Maybe Guillaume needs some help on maintaining this repo too... :)

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.

3 participants