Add ETag to static files #667
Conversation
(fn [request] | ||
(if (= [:get "/"] ((juxt :request-method :uri) request)) | ||
(if-let [resp (some-> (resource-response "index.html" {:root (or root "public") | ||
(if-let [resp (some-> (resource-response "index.html" {:root root |
danielcompton
Mar 12, 2018
Author
Contributor
I used root
here because I think the web server root is already calculated if missing at L362?
I used root
here because I think the web server root is already calculated if missing at L362?
bhauman
Mar 12, 2018
Owner
Let's leave it as it isn't part of this commit.
Let's leave it as it isn't part of this commit.
@@ -232,8 +309,8 @@ | |||
;; users handler goes last | |||
(possible-endpoint resolved-ring-handler) | |||
|
|||
(handle-static-resources http-server-root) | |||
(handle-index http-server-root) | |||
(handle-static-resources http-server-root supports-extended-attributes?) |
danielcompton
Mar 12, 2018
Author
Contributor
Do you prefer to calculate this once and pass it through to both handlers, or calculate it inside each handler?
Do you prefer to calculate this once and pass it through to both handlers, or calculate it inside each handler?
bhauman
Mar 12, 2018
Owner
I'm thinking we skip the extended attributes and try to keep this brutally simple.
I'm thinking we skip the extended attributes and try to keep this brutally simple.
bhauman
Mar 12, 2018
Owner
less code less maintainance
less code less maintainance
(defn checksum-file | ||
"Generated from Pandect" | ||
;; TODO: full attribution^^ | ||
[^File file] |
danielcompton
Mar 12, 2018
Author
Contributor
We could look at modifying this so that the file is only read once while serving it, rather than once here, and once in the response.
We could look at modifying this so that the file is only read once while serving it, rather than once here, and once in the response.
9a44b3a
to
9c76f05
This is good stuff. I think the extended attributes it probably not worth the complexity. Also, I think that every CLJS developer that has a webserver is going to want to use this code. Is there there middleware that already does this somewhere? @weavejester Developers will miss this when they switch to their own server. Always keeping in mind that we don't want to be in the server business, as there is no end the number of modifications that folks will need/want. Also are your perf numbers based on a file system that doesn't support attributes? All that aside, I'm ready to accept this commit when you are satisfied that it is working well. |
Yep, my perf numbers were for a system that didn't support attributes. I also added a simple bench test to calculate the ETag for a 45KB JS file 1000 times in each test. These were the numbers I got on CircleCI:
I just updated the test to run against a 500KB cljs.core source map file and got these results:
So on larger files (which are not all that common), the checksum can take up to about 1ms, whereas storing the checksum as a file attribute is a constant factor of about 0.03ms for all file sizes. It's still pretty small numbers either way, but I thought I'd give you that information in case that sways you one way or the other. I'm happy to pull all of the extended attribute stuff out too.
There is https://github.com/yetanalytics/ring-etag-middleware which calculates the Etag based on a SHA1 hash. I'm pretty sure a checksum would be faster, and we don't need cryptographic hashing for the ETag. I'm planning on writing a blog post about how to properly serve ClojureScript files in development, which will cover the principles here, so people know how to adapt it to their individual systems. I could turn this PR into my own Ring middleware dependency which you add into Figwheel if you'd prefer? That would help others consume it too. UPDATE: I think that would probably be good for others too if this was packaged up in a library, and then you don't have to maintain the code either. |
Yeah if you want to make this a simple middleware that figwheel consumes that would be great for everyone. |
Are you still thinking about making this separate middleware? |
Yep, just need to separate it out and publish it, will get there soon :) |
All ready for review now! |
looks great! |
For anyone interested in the future, I wrote a blog post explaining this further here: https://danielcompton.net/2018/03/21/how-to-serve-clojurescript. |
on the file to reuse it on the next request
This is still a WIP, there are a few things to work out here before merging. However I'm interested in the overall approach, namely:
Fixes #664