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

Add ETag support for StaticFile.fromUrl #7155

Open
wants to merge 1 commit into
base: series/0.23
Choose a base branch
from

Conversation

diogocanut
Copy link
Contributor

This PR brings changes made at #6621 and #4879 solving Issue #2826 .

@mergify mergify bot added series/0.23 PRs targeting 0.23.x module:core labels Jun 13, 2023
@diogocanut diogocanut mentioned this pull request Jun 13, 2023
@satorg
Copy link
Contributor

satorg commented Jun 13, 2023

Thank you for the PR!

I am a bit concerned though about using java.net's URL and URLConnection as a part of public APIs.
Is it somewhat necessary, or can it be switched to http4s Uri and maybe fs2.net capabilities ?

@armanbilge
Copy link
Member

Is it somewhat necessary

I cannot remember why at this moment, but I think for some reason it was.

or can it be switched to http4s Uri and maybe fs2.net capabilities ?

Well, yes and no 😂 in theory, absolutely yes. In practice, it would require implementing all the protocols that URLConnection supports ... such as HTTP ... and then we end up with Ember lol.

@satorg
Copy link
Contributor

satorg commented Jun 14, 2023

I mean, perhaps it could be ok to use it internally, when necessary... Just wondering, if it is possible to avoid exposing it in APIs.. Sorry, I haven't wrapped my head around the case yet, so perhaps I'm saying silly things. I just get triggered to java.net.URL and a lot of blocking calls inside 😊

@armanbilge
Copy link
Member

Not at all, it's a very good question, it's a suspicious looking API. I must have been convinced at some point, when I was reviewing the previous PR, but I'm having a hard time remembering why we needed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:core series/0.23 PRs targeting 0.23.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants