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

Files in "static" should not be cacheable #631

Closed
awwx opened this issue Jan 23, 2013 · 11 comments
Closed

Files in "static" should not be cacheable #631

awwx opened this issue Jan 23, 2013 · 11 comments
Labels

Comments

@awwx
Copy link
Contributor

@awwx awwx commented Jan 23, 2013

Files in "static" should be served with a cache control header with "max-age=0" to specify that they shouldn't be cached. (While files that we do want to be cached should be placed in "static_cacheable").

We've submitted a pull request to be able to implement this using gzippo: tomgco/gzippo#49

With this fix to gzippo, we can change

app.use(gzippo.staticGzip(path.join(bundle_dir, 'static')));

to

app.use(gzippo.staticGzip(path.join(bundle_dir, 'static'), {clientMaxAge: 0}));

Note that with this change we should also serve the development .js and .css files included in app_html out of the "static_cacheable" directory. It's OK to cache these files because they have the "cache busting" url query hash parameter, and it would make the development experience worse (for browsers that honor the cache header) to force all the packages to have to be rechecked for changes on every page load.

@n1mmy
Copy link
Member

@n1mmy n1mmy commented Jan 24, 2013

Thanks for taking care of this, @awwx! Your PR to gzippo looks great.

@awwx
Copy link
Contributor Author

@awwx awwx commented Feb 5, 2013

So it's been a couple weeks, and no response to my PR from the gzippo maintainer as of yet.

I also looked around for another npm module that we could use instead, but didn't find one that seemed as suitable (though with 20,000 npm modules I could have easily missed it :).

How do we want to move forward? gzippo is MIT-licensed, so we could wrap it in a smart package with local modifications, similar to sockjs. Thoughts?

(A fix for this caching control header issue is needed for the app cache, because the browser won't reload modified resources otherwise).

@n1mmy
Copy link
Member

@n1mmy n1mmy commented Feb 6, 2013

I think the easiest thing to do is to fork gzippo on github and build a dev-bundle with a version of gzippo from git (like we do for uglify right now). Since gzippo is used on the server, not in a package (even on the engine branch), the only changes needed should be in generate-dev-bundle.sh.

We can also implement new etag logic on this fork, for issue #626.

@awwx
Copy link
Contributor Author

@awwx awwx commented Feb 6, 2013

OK, do you want to take awwx/gzippo and copy it into meteor/gzippo?

@n1mmy
Copy link
Member

@n1mmy n1mmy commented Feb 6, 2013

Done.

@awwx
Copy link
Contributor Author

@awwx awwx commented Feb 6, 2013

Also merge the "allow-zero-client-max-age" branch into master in the meteor/gzippo version.

@n1mmy
Copy link
Member

@n1mmy n1mmy commented Feb 6, 2013

Just do what you want on awwx/gzippo and I'll pull it. Note that you can also build from a branch with the npm install <url> syntax, so it doesn't really matter if it is on master or not.

@glasser
Copy link
Member

@glasser glasser commented Feb 6, 2013

Just for fun I took a look at https://github.com/tomgco/gzippo/network

This suggests you should probably be basing our fork off of the develop branch, not master. (Also, have we tested to make sure the issues still exist on develop?)

@awwx
Copy link
Contributor Author

@awwx awwx commented Feb 6, 2013

OK, I merged allow-zero-client-max-age into master in awwx/gzippo, so that's ready to be pulled into meteor/gzippo.

Right, well, allow-zero-client-max-age is just a topic branch, so it would be better for the dev bundle not to pull from that one.

Sorry, what is it about the network graph that suggests we should fork off of develop? (All I see is that there have been commits made more recently to develop than to master).

I am mystified though; https://npmjs.org/package/gzippo says the current gzippo version is 0.2.0 but that isn't listed in https://github.com/tomgco/gzippo/tags ... I'll take another look tomorrow, maybe the published npm version is actually based off of develop.

@awwx
Copy link
Contributor Author

@awwx awwx commented Feb 6, 2013

Further investigation: Meteor's current dev_bundle is using gzippo v0.1.7. By pure luck, the master branch that I applied my fix to turns out to be the same code as v0.1.7. The only difference is the addition of a readme and some _* fields to package.json, which I'm guessing is something that happens on publishing to npm? I not finding any commits in the repo that have those additions to package.json anyway.

The result is that what we have now in meteor/gzippo master is the version of gzippo that is currently included in the dev bundle, plus the allow-zero-client-max-age fix.

Updating admin/generate-dev-bundle.sh to use the new gzippo should wait until I finish my changes to the bundler, which will include moving development .js and .css files into static_cacheable as mentioned in the last paragraph (#631 (comment))

Summary: we're good, and this issue is on hold at the moment waiting for the bundler changes.

awwx added a commit to awwx/meteor that referenced this issue Feb 7, 2013
Fixes meteor#631.

Update the dev_bundle to use the Meteor version of gzippo which allows
clientMaxAge to be set to 0, and change server.js to set the
clientMaxAge to 0 for static.
n1mmy added a commit that referenced this issue Feb 8, 2013
Fixes #631.

Update the dev_bundle to use the Meteor version of gzippo which allows
clientMaxAge to be set to 0, and change server.js to set the
clientMaxAge to 0 for static.
@n1mmy
Copy link
Member

@n1mmy n1mmy commented Feb 8, 2013

Fixed in #671. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.