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

Refactor Blaze client creation and deprecate BlazeSimpleHttp1Client #1523

Merged
merged 7 commits into from Dec 23, 2017

Conversation

@aeons
Copy link
Member

@aeons aeons commented Nov 4, 2017

This is just the simplest possible solution to #1522.

Closes #1522

blaze-client/src/main/scala/org/http4s/client/blaze/Http1Client.scala Outdated
BlazeClient(pool, config, pool.shutdown())
}

def stream[F[_]: Effect](

This comment has been minimized.

@rossabaker

rossabaker Nov 7, 2017
Member

Can these pool properties move into the config? It seems weird to pass this big old case class in after already having to set three properties.

This comment has been minimized.

@aeons

aeons Nov 9, 2017
Author Member

Yes

blaze-client/src/main/scala/org/http4s/client/blaze/package.scala Outdated
* This client will create a new connection for every request.
*/
def defaultClient[F[_]: Effect]: Client[F] = SimpleHttp1Client(BlazeClientConfig.defaultConfig)
def defaultClient[F[_]: Effect]: F[Client[F]] = Http1Client()

This comment has been minimized.

@rossabaker

rossabaker Nov 7, 2017
Member

What does this even get used for?

This comment has been minimized.

@aeons

aeons Nov 9, 2017
Author Member

Dunno, but with only one client it doesn't make much sense.

This comment has been minimized.

@aeons

aeons Nov 9, 2017
Author Member

Remove or leave with deprecation notice?

This comment has been minimized.

@rossabaker

rossabaker Nov 9, 2017
Member

I will always support deprecating changes that don't add drag for us to keep around. Makes migration easier for users.

This comment has been minimized.

@aeons

aeons Nov 9, 2017
Author Member

How about the defaultClientStream I added. Just remove it again?

blaze-client/src/main/scala/org/http4s/client/blaze/package.scala Outdated
def defaultClientStream[F[_]: Effect]: Stream[F, Client[F]] = Http1Client.stream()

@deprecated("Use Http1Client instead", "0.18.0-M6")
type PooledHttp1Client = Http1Client.type

This comment has been minimized.

@rossabaker

rossabaker Nov 7, 2017
Member

Since the signature of apply is changing, does it make more sense to leave a deprecated object instead of case class?

This comment has been minimized.

@aeons

aeons Nov 9, 2017
Author Member

What exactly do you mean? Right now it's a type alias for an object.

This comment has been minimized.

@rossabaker

rossabaker Nov 9, 2017
Member

The parameters to PooledHttp1Client.apply are different than the parameters to Http1Client. Anyone who calls PooledHttp1Client is not going to compile. If we deprecated the old one, we could point them to the new one but let them upgrade at their own pace.

This comment has been minimized.

@aeons

aeons Nov 9, 2017
Author Member

So, keep an object with apply having the same signature as the old one that just calls the new one, and with a deprecation notice? Got it :)

@aeons aeons force-pushed the aeons:blaze-client-creation branch Nov 9, 2017
blaze-client/src/test/scala/org/http4s/client/blaze/BlazeSimpleHttp1ClientSpec.scala Outdated
@@ -3,6 +3,7 @@ package org.http4s.client.blaze
import org.http4s.client.ClientRouteTestBattery
import org.http4s.util.threads.newDaemonPoolExecutionContext

@deprecated("Well, we still need to test it", "0.18.0-M6")

This comment has been minimized.

@rossabaker

rossabaker Nov 9, 2017
Member

Is this to get around a deprecation warning invoking deprecated code?

I don't think we necessarily do need to test it, if this test coverage exists against the new interface.

This comment has been minimized.

@aeons

aeons Dec 11, 2017
Author Member

The pooled client is tested, as it was before. We are still providing the simple client, although it is deprecated. This is just to keep the tests for that running.

Should the deprecated simple client instead point to a default config pooled client?

This comment has been minimized.

@rossabaker

rossabaker Dec 22, 2017
Member

Let's leave it and not change behavior, in case people are relying on it. We're already scolding the user.

@@ -50,7 +50,7 @@ speaks HTTP 1.x.
```tut:book
import org.http4s.client.blaze._

val httpClient = PooledHttp1Client[IO]()
val httpClient = Http1Client[IO]().unsafeRunSync

This comment has been minimized.

@rossabaker

rossabaker Nov 9, 2017
Member

I get what's going on here, but this runs against "unsafeRunSync only at the end of the world." But putting the entire tut into a comprehension also won't work. Maybe we need to explain ourselves here?

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Nov 10, 2017
Member

This is a hard issue as best practice would be using the streaming iterface and using it only where we need it, probably in multiple blocks calling different clients. Although in an app you generally want 1 client bracketed near the beginning.

This comment has been minimized.

@rossabaker

rossabaker Nov 10, 2017
Member

Yeah, I think in some of the other ones we can flatmap our way to victory. This one, since it's a longer narrative, is the chewy one.

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Nov 10, 2017
Member

At a minimum we need to add comments on why this was done.

@ChristopherDavenport
Copy link
Member

@ChristopherDavenport ChristopherDavenport commented Nov 25, 2017

ping @aeons

Do you have any time to update the comment for client.md? If not I'll push that up for you as I think we're ready to approve otherwise.

@aeons
Copy link
Member Author

@aeons aeons commented Nov 25, 2017

Completely forgot this again. I’m completely booked this weekend, so you can just push what you want.

@aeons
Copy link
Member Author

@aeons aeons commented Dec 11, 2017

Right now, there's an apply that gives you back an F[Http1Client[F]], and a stream method that uses bracket to give you back a Stream[F, HttpClient[F]].

The apply just wraps creation in F.delay.

If you guys are ok with those two, we could add an unsafeCreate, that returns a raw Http1Client[F], and use that in docs with a note that you should probably use the stream variant?

@aeons aeons force-pushed the aeons:blaze-client-creation branch to b174a39 Dec 22, 2017
@jmcardon jmcardon merged commit bf32d9a into http4s:master Dec 23, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@aeons aeons deleted the aeons:blaze-client-creation branch Dec 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.