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

ChunkAggregator for HttpRoutes and HttpApp #2429

Conversation

Projects
None yet
3 participants
@voidcontext
Copy link
Contributor

voidcontext commented Feb 20, 2019

This PR adds convenience shortcuts to the ChunkAggregattor to apply the middleware on HttpRoutes and HttpApps.

Related issue: #2402

@voidcontext voidcontext changed the title WIP - ChunkAggregator for HttpRoutes and HttpApp ChunkAggregator for HttpRoutes and HttpApp Feb 22, 2019

@rossabaker
Copy link
Member

rossabaker left a comment

Thanks!

This is going to get me in trouble with the OOphobes, but couldn't we have a trait for the middleware objects that provides this for all the middleware of this shape?

@rossabaker

This comment has been minimized.

Copy link
Member

rossabaker commented Feb 23, 2019

The reason I keep bringing up traits is that I still haven't seen another maintainable technique that doesn't add an import tax to the user. If typeclass syntax on the singleton types went into the implicits that are already imported, fine, but this would be implicits in another package, as well as a lawless typeclass. And I fail to see how it's worse than copy and paste.

@voidcontext

This comment has been minimized.

Copy link
Contributor Author

voidcontext commented Feb 23, 2019

@rossabaker I have a feeling I've missed some of the earlier discussions around this topic. Could you please explain what you mean when you say "but this would be implicits in another package, as well as a lawless typeclass" ?

In general I agree, it would be nice to give the middlewares a structure, where these kinds of shortcuts could be implemented in a general way. Let me dive into this and do some experiments.

Meanwhile I am happy to park this PR until we can reach consensus.

@rossabaker

This comment has been minimized.

Copy link
Member

rossabaker commented Feb 23, 2019

I think we can merge this because we don't have a consensus on a better way forward and have been arguing about this pattern for months. But I'd still like to solve this general problem in an agreeable way, because it keeps coming up.

@voidcontext

This comment has been minimized.

Copy link
Contributor Author

voidcontext commented Feb 26, 2019

As far as I can see there's already a middleware type:

type Middleware[F[_], A, B, C, D] = Kleisli[F, A, B] => Kleisli[F, C, D]

Which is good, but not really used in the code. Looking at the current middleware implementations it seems in most of the cases either the request or the response is fixed, sometimes both (Http[F, G]). We could introduce something like type RequestAwareMiddleware[F, G, B] = Kleisli[F, Request[G], B] and ResponseAwareMiddleware the same way, then the current implementations could return e,g, Reader[RequestAwareMiddleware[F, G, B], RequestAwareMiddleware[F, G, B]] (or maybe even without the reader, but just a plain function), which we could extend with implicit classes to add the necessary syntax (shortcuts). Disclaimer: I didn't check if this could be implemented, or if it would cover all the cases. Just an idea.

Probably not for the upcoming 0.20 release because of backwards compatibility, but if this is something worth to explore for future releases, I'd happily build a POC.

@rossabaker

This comment has been minimized.

Copy link
Member

rossabaker commented Feb 28, 2019

I was thinking it couldn't be done without adding to the import tax, but I think that's wrong. Even though the middlewares all live in server or client, the shape of middleware is core, and could be in the org.http4s.implicits._ that many people already implement.

I am not entirely comfortable adding syntax to general types to support things that aren't typeclasses, but that's exactly what we've done with .orNotFound.

I don't have a strong opinion. Does anyone else?

@rossabaker

This comment has been minimized.

Copy link
Member

rossabaker commented Feb 28, 2019

I think we should open a separate issue, because there was consensus to move forward with this.

@rossabaker rossabaker merged commit 027916b into http4s:master Feb 28, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.