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

don't read to file when the browser cache is valid #69

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

amitport
Copy link

No description provided.

@haoxins
Copy link
Member

haoxins commented Feb 14, 2017

@amitport
Copy link
Author

@coderhaoxin, wouldn't conditional-get still cause the file to be read?

So although the client wouldn't wait for the server to read the file, the server will become slow when trying to process many requests in parallel

@haoxins
Copy link
Member

haoxins commented Feb 15, 2017

Personally, I have CDN for production.

/cc @jonathanong

@jonathanong
Copy link
Member

yes, I try not to serve files directly on my node.js server. they should be served via CDN.

However, I understand how the developer UX for this modular is subpar. When TJ and I were making this, I wasn't too fond of the decision to not bake the caching logic into the middleware as, as you said, it creates a file descriptor. but it does make the code simpler.

IMO it's up to the community—I don't mind baking in conditional get logic here or not. I care more that coverage dropped :)

@haoxins
Copy link
Member

haoxins commented Feb 15, 2017

@amitport can you add some tests for this ? :)

@amitport
Copy link
Author

amitport commented Feb 15, 2017

@coderhaoxin no problem, I will add this behind an optional flag and add docs+tests in a few days.

Regading CDN, IMHO, koa should provide something reasonable out-of-the-box and this is just common sense http, not some crazy optimization.

@magicdawn
Copy link

+1 on this

@haoxins haoxins self-assigned this Apr 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants