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

Change Effect to Sync in FileService #3124

Merged
merged 1 commit into from Feb 3, 2020
Merged

Change Effect to Sync in FileService #3124

merged 1 commit into from Feb 3, 2020

Conversation

erdeszt
Copy link
Contributor

@erdeszt erdeszt commented Jan 28, 2020

The extra features provided by Effect are not used and relaxing the constraints makes it easier to use it with ZIO instead of IO.

Edit: I'm a bit confused about the failing spec. Seems unrelated to my change but I don't have much experience with the test suite so I could be wrong.

Copy link
Member

@rossabaker rossabaker left a comment

That test is relatively new and has been flaky. I restarted.

@@ -6,7 +6,7 @@ import cats.effect._

/** Cache strategy that doesn't cache anything, ever. */
class NoopCacheStrategy[F[_]] extends CacheStrategy[F] {
override def cache(uriPath: String, resp: Response[F])(implicit F: Effect[F]): F[Response[F]] =
override def cache(uriPath: String, resp: Response[F])(implicit F: Sync[F]): F[Response[F]] =
Copy link
Member

@rossabaker rossabaker Jan 28, 2020

Choose a reason for hiding this comment

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

Isn't this one just Applicative?

Copy link
Member

@rossabaker rossabaker Jan 28, 2020

Choose a reason for hiding this comment

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

Ah, no, because of the trait. Perhaps we could move this constraint to a constructor for the strategy.

Copy link
Contributor Author

@erdeszt erdeszt Jan 28, 2020

Choose a reason for hiding this comment

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

You mean remove it from CacheStrategy.cache? Should I do it in this PR?

Copy link
Member

@rossabaker rossabaker Jan 29, 2020

Choose a reason for hiding this comment

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

If it's on CacheStrategy.cache, it has to be on all the implementations. I'm thinking of moving the constraints to the MemoryCache and NoopCacheStrategy constructors. MemoryCache would require Sync, and NoopCacheStrategy would be fine with just Applicative.

... at least that's how it compiles in my head.

Copy link
Contributor Author

@erdeszt erdeszt Jan 29, 2020

Choose a reason for hiding this comment

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

I've started on it but ran into an issue because NoopCacheStrategy is used as a default argument in FileService.Config.apply(https://github.com/http4s/http4s/blob/master/server/src/main/scala/org/http4s/server/staticcontent/FileService.scala#L38) and that doesn't work if I have the constraint is on the constructor.

I could make the cache strategy a non optional argument but I'm not sure if that's the right call.

Copy link
Member

@rossabaker rossabaker Jan 29, 2020

Choose a reason for hiding this comment

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

I prefer builder patterns for configuration, because we can add more configuration options later without breaking bincompat. But that's still ugly with regard to this problem: we would need to supply the constraint wherever the default value is materialized.

And getting the strategy to Applicative doesn't even do much in practice, because the service that uses it still requires Sync.

I think maybe we accept this as incremental progress and rethink it for 1.0.

@rossabaker rossabaker added the breaking label Jan 28, 2020
@rossabaker rossabaker added this to the 0.21.0-RC3 milestone Jan 28, 2020
hamnis
hamnis approved these changes Feb 3, 2020
@rossabaker rossabaker merged commit e7ccce1 into http4s:master Feb 3, 2020
2 checks passed
@erdeszt erdeszt deleted the relax-fileservice-constraints branch Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants