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

Package structure: org.http4s.$role.$backend or org.http4s.$backend.$role #2702

Closed
rossabaker opened this issue Jul 9, 2019 · 22 comments
Closed
Assignees
Labels
good first issue A tractable issue for those looking to make an initial contribution module:client module:server
Milestone

Comments

@rossabaker
Copy link
Member

This discussion originated in the ember PR, but we didn't have a strong consensus. Ember is org.http4s.ember.client. Blaze (and all other backends) are org.http4s.client.blaze. There are good reasons for both, but I think consistency is better than either argument. Let's standardize on one.

Quoting a fool:

One thing that we all got right is to have both $backend and (server | client) in the package name. The Jetty server is older than the Jetty client, but we were able to add the client without moving the server later. So it's really a matter of which comes first. I'd like us to be consistent. I'm willing to consider deprecating and moving the other backends with a scalafix.

  1. There's more package-private code in ember than there is in server and client, so ember benefits from this convention. The alternative is to leave ember-core in org.http4s.ember, but make everything private[http4s].
  2. There's also quite a bit in blaze, which could benefit from backend-first. But org.http4s.blaze is the blaze project's package, so we need to be very careful about polluting that namespace. We've solved that today with a blazecore for the shared code, all private[http4s]. We could maybe move non-http4s blaze out of org.http4s, but to where? That's a lot of moving for something nobody has complained about.
  3. There's no shared http4s-jetty code between server and client. If there were, the ember convention would be a little nicer.
  4. No other backend has both a server and client. These can all benefit slightly from the current convention, with package-private server and client helpers. But the one and only that exists right now is DefaultClient.

Points 3 and 4 are weak. 1 is a good argument for change. 2 is a good argument for the status quo.

@rossabaker rossabaker added the RFC Design ideas that we'd like to spur discussion label Jul 9, 2019
@rossabaker rossabaker added this to the 1.0 milestone Apr 6, 2021
@rossabaker
Copy link
Member Author

If ember stands its ground, we'll need to scalafix the heck out of a lot of other things.

@rossabaker
Copy link
Member Author

I can't find a single instance of us sharing private server or client components between backends, but many instances in blaze and ember of sharing backend components between server and client. That suggests $backend.$role.

$backend will also more naturally reflect our repo organization, where org.http4s.$backend usually lives in http4s-$backend repo.

I think we should move in this direction, and I know @ChristopherDavenport will be happy.

@rossabaker
Copy link
Member Author

#4702 is a good template for this, and we could use some help.

@rossabaker rossabaker added good first issue A tractable issue for those looking to make an initial contribution and removed RFC Design ideas that we'd like to spur discussion labels Apr 14, 2021
@rossabaker
Copy link
Member Author

rossabaker commented Apr 14, 2021

  • client.asynchttpclient -> asynchttpclient.client
  • client.blaze -> blaze.client
  • server.jetty -> jetty.server
  • client.jetty -> jetty.client
  • client.okhttp -> okhttp.client
  • server.tomcat -> tomcat.server

@rossabaker
Copy link
Member Author

This also has ramifications for the http4s-jdk-http-client project. It doesn't have to change, but it would fit better with the rest.
/cc @amesgen.

Armeria, Finagle, and Netty all appear to be under the new convention.

@rossabaker rossabaker modified the milestones: 1.0, 0.22 Apr 14, 2021
@rossabaker
Copy link
Member Author

We're targeting series/0.22 with this. Please leave a comment if you're grabbing one.

@vasilmkd
Copy link
Contributor

I'd like to work on server.jetty -> jetty.server.

@amesgen
Copy link
Member

amesgen commented Apr 14, 2021

Will do this for http4s-jdk-http-client in 0.4 and 0.5 👍

@rossabaker
Copy link
Member Author

@amesgen I wonder whether we should add a scalafix here? That's a little weird to cross repos like that, but it's all binary dependencies in the scalafix project, so it's not really cyclic.

@vasilmkd
Copy link
Contributor

I'll pick up client.jetty -> jetty.client too. Does the base jetty directory need to be changed to jetty-server?

@rossabaker
Copy link
Member Author

Yes! #4744 for that.

@vasilmkd
Copy link
Contributor

vasilmkd commented Apr 14, 2021

Doing okhttp and asynchttpclient.

@vasilmkd
Copy link
Contributor

vasilmkd commented Apr 16, 2021

Just to confirm, we still need client.blaze -> blaze.client and server.blaze -> blaze.server right?

@rossabaker
Copy link
Member Author

Yep. It would also make sense to move the blazecore package, but that risks conflicting with the upstream blaze repo. We'll be moving this blaze backend back into that repo, so maybe we can sort out blazecore then. It should all be private[http4s] anyway.

@kry00la
Copy link
Contributor

kry00la commented May 13, 2021

Is client.blaze -> blaze.client taken? I would like to take it.

@vasilmkd
Copy link
Contributor

Feel free to work on it @krayoola 😄

@rossabaker
Copy link
Member Author

What's left? blaze-server, which I missed in the initial checklist? Anything else?

@amesgen
Copy link
Member

amesgen commented May 18, 2021

Ah, I started this but then got confused about the concrete module name. Is it:

  • org.http4s.jdkhttpclient.client
  • org.http4s.jdkhttp.client

? Right now, it is org.http4s.client.jdkhttpclient.

@vasilmkd
Copy link
Contributor

We have the same problem with asynchttpclient I believe. Let's try to come to a consensus on the issue. I am honestly indifferent.

@kry00la
Copy link
Contributor

kry00la commented May 18, 2021

I'm taking blaze-server

@hamnis
Copy link
Contributor

hamnis commented May 19, 2021

I have some naming suggestions:

@amesgen org.http4s.jdkhttpclient or org.http4s.jdk.client
@vasilmkd org.http4s.asynchttpclient or org.http4s.ahc.client

@rossabaker
Copy link
Member Author

I'd go with jdkhttpclient and asynchttpclient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue A tractable issue for those looking to make an initial contribution module:client module:server
Projects
None yet
Development

No branches or pull requests

5 participants