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

Port MIME type detection over from HttpServer.jl #68

Merged
merged 3 commits into from
Aug 22, 2018
Merged

Port MIME type detection over from HttpServer.jl #68

merged 3 commits into from
Aug 22, 2018

Conversation

rdeits
Copy link
Collaborator

@rdeits rdeits commented Aug 21, 2018

Fixes JuliaGizmos/WebIO.jl#181

This was broken in the transition from HttpServer.jl to HTTP.jl. There's a TODO in the code to switch to HTTP.sniff() but that doesn't quite work. The problem is that HTTP.sniff() only operates on the file contents and not the extension, so it always returns text/plain for things like .css files, while old implementation correctly returned text/css.

To avoid a new dependency, I just copied over the whole mimetypes.jl file from HttpServer into Mux. It's not an elegant solution, but given that the current behavior is broken, I think it's necessary. In the future, we could consider moving the mimetype list elsewhere.

@rdeits
Copy link
Collaborator Author

rdeits commented Aug 21, 2018

Here's the old implementation that I'm essentially restoring:

fileheaders(f) = d("Content-Type" => get(mimetypes, extension(f), "application/octet-stream"))

@codecov-io
Copy link

codecov-io commented Aug 21, 2018

Codecov Report

Merging #68 into master will increase coverage by 0.34%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #68      +/-   ##
=========================================
+ Coverage   90.56%   90.9%   +0.34%     
=========================================
  Files           5       6       +1     
  Lines          53      55       +2     
=========================================
+ Hits           48      50       +2     
  Misses          5       5
Impacted Files Coverage Δ
src/Mux.jl 100% <ø> (ø) ⬆️
src/examples/files.jl 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 962b54f...4bbd9e5. Read the comment docs.

@piever
Copy link
Collaborator

piever commented Aug 21, 2018

Maybe this could be moved to HTTP eventually, but for now this PR is a good practical solution.

@hustf
Copy link

hustf commented Aug 21, 2018

Thank you for the great work you are putting in. This may belong in a new HttpCommon in the end, but for now I will refer Mux.jl in private repositories.

Would you consider including some additional extensions? I always add these lines to HttpServer (sorry for layout)

Add some mime types, which some browsers support.

HttpServer.mimetypes["jl"]= "text/plain"
HttpServer.mimetypes["md"] = "text/markdown; charset=UTF-8"
HttpServer.mimetypes["vec"] = "text/plain"

@rdeits
Copy link
Collaborator Author

rdeits commented Aug 22, 2018

I don't actually know much about the details of MIME types. The .jl one seems fine, but why is the charset specified only for markdown? And what is a .vec file? There seem to be a bunch of formats using that extension, some of which are binary.

Maybe we can decide whether to add those as part of a separate PR? I'd like to get the original issue fixed as soon as possible.

@piever
Copy link
Collaborator

piever commented Aug 22, 2018

Maybe we can decide whether to add those as part of a separate PR? I'd like to get the original issue fixed as soon as possible.

Yes! Not loading any css library right now in WebIO is pretty bad, I think we can discuss in a separate PR which other mimetype detection we want to add and whether we want to allow a mechanism to customize this detection on the user side.

@hustf
Copy link

hustf commented Aug 22, 2018

Not adding these is of course just fine. The more one adds, the less general the code becomes.

@rdeits rdeits merged commit 7fdabed into master Aug 22, 2018
@rdeits rdeits deleted the rd/mime branch August 22, 2018 22:42
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.

4 participants