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

Merge pull request #3052 from ChristopherDavenport/cachingHeadersMiddleware #3052

Conversation

ChristopherDavenport
Copy link
Member

@ChristopherDavenport ChristopherDavenport commented Jan 9, 2020

No description provided.

@ChristopherDavenport ChristopherDavenport marked this pull request as ready for review Jan 10, 2020
Copy link
Member

@rossabaker rossabaker left a comment

Nice. Looks almost ready.

}
}

object Helpers {
Copy link
Member

@rossabaker rossabaker Jan 10, 2020

Choose a reason for hiding this comment

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

Does this need to be public? If it does, I might consider moving the two helpers herein to the top-level. Things nested in Helpers or Utils or bits tend to not be discoverable.

Copy link
Member Author

@ChristopherDavenport ChristopherDavenport Jan 10, 2020

Choose a reason for hiding this comment

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

The thing I was hoping to achieve was to allow those who used caching to have a way to get the tools we use rather than recreate them, but only leave the middlewares exposed at the top level.

Copy link
Member

@rossabaker rossabaker Jan 12, 2020

Choose a reason for hiding this comment

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

Okay, fine with me as is.

case finite: FiniteDuration => finite
case _ => 315360000.seconds // 10 years
// Http1 caches do not respect max-age headers, so to work globally it is recommended
// to explicitly set an Expire which requires some time interval to work
Copy link
Member

@rossabaker rossabaker Jan 10, 2020

Choose a reason for hiding this comment

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

Nice comment. I was getting ready to throw a fit about Duration about 20 lines above.

}

def publicCache[G[_]: MonadError[*[_], Throwable]: Clock, F[_]](
lifetime: Duration,
Copy link
Member

@rossabaker rossabaker Jan 10, 2020

Choose a reason for hiding this comment

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

I feel like we more commonly put the Http alone in a parameter list, for partial application. But I got that feeling a long time ago, before we had all the G and what not. I'm not sure whether that's still a useful convention.

Copy link
Member Author

@ChristopherDavenport ChristopherDavenport Jan 10, 2020

Choose a reason for hiding this comment

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

We could put it as the first argument, but if we put it in the second like we generally do, then G and F cannot infer

Copy link
Member

@rossabaker rossabaker Jan 12, 2020

Choose a reason for hiding this comment

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

Drat. I think you're right. That's sad, because I like to think of these as partially applied up to the Http.

Copy link
Member Author

@ChristopherDavenport ChristopherDavenport Jan 12, 2020

Choose a reason for hiding this comment

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

Alternative is building it as partially applied class. So that we can say Duration -> (forall g, f . ((MonadError Throwable g), Clock g) => (http g f) -> (http g f))

Copy link
Member

@rossabaker rossabaker Jan 13, 2020

Choose a reason for hiding this comment

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

Ooh, that's an interesting idea, if the type inference still works well enough when folding them down into one.

.map(Instant.ofEpochMilli(_))
.flatMap(nowInstant => HttpDate.fromInstant(nowInstant).liftTo[G])
.map(nowHttpDate => HDate(nowHttpDate))
HttpDate.current[G].map(HDate(_))
Copy link
Member

@rossabaker rossabaker Jan 10, 2020

Choose a reason for hiding this comment

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

To our future selves: we've usually resisted putting convenience constructors directly on headers, but things like this make me wonder.

Copy link
Member

@rossabaker rossabaker left a comment

👍

}
}

object Helpers {
Copy link
Member

@rossabaker rossabaker Jan 12, 2020

Choose a reason for hiding this comment

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

Okay, fine with me as is.

}

def publicCache[G[_]: MonadError[*[_], Throwable]: Clock, F[_]](
lifetime: Duration,
Copy link
Member

@rossabaker rossabaker Jan 12, 2020

Choose a reason for hiding this comment

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

Drat. I think you're right. That's sad, because I like to think of these as partially applied up to the Http.

@ChristopherDavenport ChristopherDavenport changed the title Add Caching Headers Middleware Merge pull request #3052 from ChristopherDavenport/cachingHeadersMiddleware Jan 13, 2020
@ChristopherDavenport ChristopherDavenport merged commit ec62b48 into http4s:master Jan 13, 2020
2 checks passed
@ChristopherDavenport ChristopherDavenport deleted the cachingHeadersMiddleware branch Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants