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

cats-mtl #4758

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

cats-mtl #4758

wants to merge 19 commits into from

Conversation

rossabaker
Copy link
Member

A cleaner version of #4754 now that I have a better idea what I'm doing.

apply(httpRoutes)

def httpApp[F[_]: Applicative](httpApp: HttpApp[F]): HttpApp[F] =
def httpApp[F[_]](httpApp: HttpApp[F])(implicit F: Monad[F]): HttpApp[F] =
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you strengthen this constraint?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because apply needs a Monad now, and Kleisli doesn't have one unless F has one.

@rossabaker
Copy link
Member Author

There are several examples here. I did cowardly hide from a couple of the more difficult ones. I think it's enough to decide whether we want to proceed with this and try to attract new contributors, or stick with what we have.

@rossabaker rossabaker added module:client module:server RFC Design ideas that we'd like to spur discussion breaking Change breaks compatibility (binary or source) labels Apr 18, 2021
@rossabaker rossabaker added this to the 0.22 milestone Apr 18, 2021
@rossabaker
Copy link
Member Author

It doesn't make much sense to release half the middlewares in this style. Unless someone has a good reason otherwise, I think this can slip to 1.0.

@rossabaker rossabaker modified the milestones: 0.22, 1.0 May 24, 2021
@rossabaker rossabaker changed the base branch from series/0.22 to main May 24, 2021 01:48
@rossabaker
Copy link
Member Author

I think this proves the techinque, and we could recruit help to get the rest done.

@rossabaker rossabaker marked this pull request as ready for review May 27, 2021 20:46
@rossabaker rossabaker removed the RFC Design ideas that we'd like to spur discussion label May 27, 2021
@bplommer
Copy link
Member

bplommer commented Aug 12, 2021

Much cleaner! Is the plan still to do this for 1.0? I'm happy to lend a hand if so.

@rossabaker
Copy link
Member Author

I'm still in favor of this and regret that we didn't finish before 0.23. I would love the help! How about I reserve the merge conflicts and get this approved to ensure we're not the only two who like it, and then I can hand it off to you?

@bplommer
Copy link
Member

How about I reserve the merge conflicts and get this approved to ensure we're not the only two who like it, and then I can hand it off to you?

Sure, with the caveat that I don't take on any feeling of responsibility or obligation 😉

How about merging this once approved, opening an issue, and merging new ones as they're ready? That way we avoid new merge conflicts.

@rossabaker
Copy link
Member Author

Agreed. I got a bit carried away implmenting them while proving to myself it was good, and then gathering support for the idea. One-by-one would be much better moving forward.

@Daenyth
Copy link
Contributor

Daenyth commented May 18, 2022

I haven't read through the whole PR history so perhaps I missed it, but what is the goal of these changes?
What user experience / feature offering is improved by it?

I'm very concerned with the kind of type signatures appearing in the diff here. Currently in my experience, http4s has consistently been the single library worst for new-to-scala people or new-to-fp people, of libraries that are in common use across the tech stack. I'm afraid that this change would reduce the accessibility even more. I love using the library because it has a ton of nice properties, but this feels like a complexity increase that I'm not sure is fully justified. It could make me reluctant to continue recommending it to scala teams that aren't already waist-deep in FP scala

@isomarcte
Copy link
Member

@Daenyth can you elaborate on that more?

To me this mostly looks like the effects of this are changing functions which were formerly explicitly Kleisli and making them have an Ask or Local constraint.

Are you worried just the presence of the new constraints will be confusing?

@SystemFw
Copy link
Member

I haven't read through the whole PR history so perhaps I missed it, but what is the goal of these changes?

Said this on Discord but it's better to keep the discussion here: I think a big motivating goal for these changes is that whilst final tagless is harder to work with than concrete IO, (the assumption is that) the reverse is true for Kleisli, which is relevant when writing middlewareswhere the Kleisli-ness cannot be otherwise ignored.
And to be honest I have to say, I know a lot competent FP programmers that have no issues writing their systems in cats-effect, but are still absolutely mistified by Kleisli.

@rossabaker
Copy link
Member Author

Kleisli continues to be controversial. MTL works both for Kleisli or for more HTTP-specific constructions. A better discussion is at #4846.

@bplommer
Copy link
Member

bplommer commented May 19, 2022

On the topic of difficult-to-parse type signatures, one thing that might help is to use more meaningful names for type parameters - for example the constructor for Autoslash could change from

def apply[F[_], G[_], B](fb: F[B])(implicit
      F: Monad[F],
      FMK: MonoidK[F],
      L: Local[F, Request[G]]
  ): F[B]

to the following, which is much more self-explanatory:

def apply[Rt[_], F[_], Resp](fb: Rt[Resp])(implicit
      Rt: Monad[Rt],
      RtMK: MonoidK[Rt],
      L: Local[Rt, Request[F]]
  ): Rt[Resp]

@rossabaker
Copy link
Member Author

What is Rt? "Routes"?

@bplommer
Copy link
Member

What is Rt? "Routes"?

Yep, wanted something more meaningful than F but still sufficiently cryptic to not get mistaken for a concrete type. Specific naming is subject to bikeshedding of course.

@bplommer
Copy link
Member

What's the next step on this, just pull the chances out into individual PRs that can be merged one by one?

@rossabaker
Copy link
Member Author

I think splitting it up is the way, but before we invest more in it:

  1. Are the dissenters satisfied? It was controversial in the day, and got a particularly strong negative reaction just yesterday.

  2. What is the target of trait HttpRoutes[F[_]] #4846? (/cc @kubokoz) Dealiasing HttpApp and HttpRoutes from Kleisli into their own things? Giving it new names to minimize breakage but risking Too Many Ways To Do It? Status quo? I'm not in favor of MTL if we keep the status quo, because MTL is an abstraction over non-Kleisli ways to do it.

@armanbilge armanbilge mentioned this pull request May 20, 2022
@armanbilge
Copy link
Member

I have a question about this. I'm curious about the choice between using Local[Rt, Request[F]] vs our own typeclass e.g. LocalRequest[F] (or something, subject to bikeshed).

@armanbilge
Copy link
Member

I guess I'm confused if we why if we really need both F{_] and G{_]. If one is IO and the other is Kleisli, can't we just use Kleisli for both?

@bplommer
Copy link
Member

I guess I'm confused if we why if we really need both F{_] and G{_]. If one is IO and the other is Kleisli, can't we just use Kleisli for both?

I don't think I understand this - can you give an example of what the signature would look like?

@armanbilge
Copy link
Member

armanbilge commented May 20, 2022

Lol, seems I was stumbling over my ifs and whys there 😅

For example, in feral we have the general shape (Event, Context[F]) => Response for AWS lambdas represented MTL style like this:

trait LambdaEnv[F[_], Event] { outer =>
  def event: F[Event]
  def context: F[Context[F]]

  final def mapK[G[_]: Functor](f: F ~> G): LambdaEnv[G, Event] =
    new LambdaEnv[G, Event] {
      def event = f(outer.event)
      def context = f(outer.context).map(_.mapK(f))
    }
}

https://github.com/typelevel/feral/blob/c1894f7276ff79eb9c028c95297bd06b089af83e/lambda/shared/src/main/scala/feral/lambda/LambdaEnv.scala#L32

@armanbilge
Copy link
Member

Ok, so I played out my idea a bit. My hope was that something like F[_]: LocalRequest would be a less scary signature. But, very quickly run into difficulties implementing local :)

//> using scala "2.13.8"
//> using plugin "org.typelevel:::kind-projector:0.13.2"
//> using lib "org.http4s::http4s-core::1.0.0-M32"
//> using lib "org.typelevel::cats-mtl::1.2.0"

import cats._
import cats.data._
import cats.mtl._
import cats.syntax.all._
import org.http4s._

trait AskRequest[F[_]] extends Ask[F, Request[F]]
trait LocalRequest[F[_]] extends AskRequest[F] with Local[F, Request[F]]

object AskRequest {

  implicit def forKleisli[F[_]](implicit F: Monad[F]): LocalRequest[Kleisli[F, Request[F], *]] =
    new LocalRequest[Kleisli[F, Request[F], *]] {
      private type G[A] = Kleisli[F, Request[F], A]

      def applicative = Applicative[Kleisli[F, Request[F], *]]

      def ask[E2 >: Request[Kleisli[F, Request[F], *]]]: Kleisli[F, Request[F], E2] =
        Kleisli.ask[F, Request[F]].map(_.mapK(Kleisli.liftK[F, Request[F]])).widen

      def local[A](fa: Kleisli[F, Request[F], A])(
          f: Request[Kleisli[F, Request[F], *]] => Request[Kleisli[F, Request[F], *]]
      ): Kleisli[F, Request[F], A] =
        ???
    }

}

@TimWSpence
Copy link
Contributor

Are there performance implications for this? Or will JIT make it all go away as they're all monomorphic callsites (I think)?

@rossabaker
Copy link
Member Author

That is a great question that I have not measured to adequately answer.

Will also be hard to benchmark, because whatever impact we see isolating it may not matter in the scope of an application dressed in full TCP socket regalia.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Change breaks compatibility (binary or source) module:blaze-client module:client module:server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants