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

Update zio-http to RC4 & Play to 3.0.0 #1968

Merged
merged 18 commits into from
Dec 7, 2023
Merged

Conversation

kyri-petrou
Copy link
Collaborator

@kyri-petrou kyri-petrou commented Oct 31, 2023

Tapir has a new release with support for zio-http RC3. This required very few changes in the adapter, but had to change the examples to work with the new routes structure

@kyri-petrou
Copy link
Collaborator Author

@ghostdogpr would it be possible to merge #1967 prior to this one? I'd like to try out those changes without migrating all the zio-http code 😅

ZHttpAdapter.makeHttpService(HttpInterpreter(interpreter))
}
Routes(
Method.ANY / "api" / "graphql" -> ZHttpAdapter.makeHttpService(HttpInterpreter(interpreter)).toHandler
Copy link
Owner

Choose a reason for hiding this comment

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

Should we return a Handler if we have to do toHandler to use it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only thing about it is that I'm unsure whether there might be other methods of composition that I haven't figured out yet. E.g., the HttpApp is also convertible to Routes

Copy link
Owner

Choose a reason for hiding this comment

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

But we're never going to use it as Routes because it doesn't have any path in it, right?

Maybe make makeHttpService return a Handler for the most common case but keep a variant that returns the HttpApp?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah after thinking a bit about it you're right, I don't think there's much of a reason returning an HttpApp. I changed it to return Handlers instead

@kyri-petrou kyri-petrou changed the title Update zio-http to RC3 Update zio-http to RC3 & Play to 3.0.0 Nov 2, 2023
@kyri-petrou
Copy link
Collaborator Author

@ghostdogpr tapir 1.8.5 is out and it adds the ability to provide custom WS config for zio-http. Have a look at the changes in ZHttpAdapter and let me know what you think.

I also added the play update in this PR otherwise CI would fail since tapir 1.8.5 is not compatible with play 2.x

@kyri-petrou kyri-petrou added the breaking change The PR contains binary incompatible changes label Nov 19, 2023
@kyri-petrou kyri-petrou changed the title Update zio-http to RC3 & Play to 3.0.0 Update zio-http to RC4 & Play to 3.0.0 Dec 1, 2023
@kyri-petrou
Copy link
Collaborator Author

PR updated to migrate the QuickAdapter to RC4

.collectHttp[Request] { case _ -> Root / "api" / "graphql" =>
ZHttpAdapter.makeHttpService(HttpInterpreter(interpreter))
}
Routes(
Copy link
Owner

Choose a reason for hiding this comment

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

Not directly related but how about using the quick adapter for federation examples (or any examples that are not about a specific adapter).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah good point. I'll change it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So much cleaner now 😃

@kyri-petrou kyri-petrou merged commit f50b6d2 into series/2.x Dec 7, 2023
10 checks passed
@kyri-petrou kyri-petrou deleted the zio-http-3.0.0RC3 branch December 7, 2023 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change The PR contains binary incompatible changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants