Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Corrects bug with using compress and staticCache together. #458

Closed
wants to merge 2 commits into from

3 participants

@ryanrolds

No description provided.

@tj tj commented on the diff
test/staticCacheCompress.js
@@ -0,0 +1,61 @@
+
+var connect = require('../');
+
+var fixtures = __dirname + '/fixtures';
+
+var app = connect();
+app.use(connect.compress());
+app.use(connect.staticCache());
+app.use(connect.static(fixtures));
+
+describe('connect.staticCache() & connect.compress()', function(){
@tj Owner
tj added a note

could we move these to test/staticCache.js as describe('Vary', or something similar

Yes, but do we want to mix the tests? Right now the test for staticCache doesn't use the compress middleware.

@tj Owner
tj added a note

nvm i thought these were for negotiating via Vary at a glance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/middleware/staticCache.js
((5 lines not shown))
+ // needed to deal with compress conflict
+ Object.keys(header).forEach(function(field) {
+ res.setHeader(field, header[field]);
+ });
+
+ res.writeHead(200);
@tj Owner
tj added a note

node defaults it to 200 so we shouldn't need this

Good point. I will take care of that.

Just updated the pull with a commit that removes the call to writeHead()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/middleware/staticCache.js
((5 lines not shown))
+ // needed to deal with compress conflict
+ Object.keys(header).forEach(function(field) {
+ res.setHeader(field, header[field]);
+ });
@tj Owner
tj added a note

i might be missing something but wouldn't this be pretty much identical to the old res.writeHead call?

One major difference, it doesn't trigger the header event. When the header event is fired, compress checks the headers but the new headers (specifically the Content-Type) haven't been set yet. The compress middlware doesn't compress if the Content-Type is missing.

@tj Owner
tj added a note

hmm every response should have a "header" event at some point, but that's a connect thing in patch.js, if it doesn't it's a bug

Yeah, I saw it in patch.js. The issue is the header event is emitted before the headers in arguments[1] are applied, so when the compress middleware tries to get the Content-Type header in filter() it doesn't exist yet.

@tj Owner
tj added a note

right ok I think I get what you mean now. That's what really blows about node's different APIs for the same task haha. If writeHead() used setHeader internally we'd probably be fine but I gotcha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 15, 2012
  1. @ryanrolds

    Fixed conflict with compress and staticCache that was preventing cach…

    ryanrolds authored
    …ed items from being compressed
  2. @ryanrolds
This page is out of date. Refresh to see the latest.
Showing with 66 additions and 1 deletion.
  1. +5 −1 lib/middleware/staticCache.js
  2. +61 −0 test/staticCacheCompress.js
View
6 lib/middleware/staticCache.js
@@ -147,7 +147,11 @@ module.exports = function staticCache(options){
// respond with cache
header['x-cache'] = 'HIT';
- res.writeHead(200, header);
+
+ // needed to deal with compress conflict
+ Object.keys(header).forEach(function(field) {
+ res.setHeader(field, header[field]);
+ });
// backpressure
function write(i) {
View
61 test/staticCacheCompress.js
@@ -0,0 +1,61 @@
+
+var connect = require('../');
+
+var fixtures = __dirname + '/fixtures';
+
+var app = connect();
+app.use(connect.compress());
+app.use(connect.staticCache());
+app.use(connect.static(fixtures));
+
+describe('connect.staticCache() & connect.compress()', function(){
@tj Owner
tj added a note

could we move these to test/staticCache.js as describe('Vary', or something similar

Yes, but do we want to mix the tests? Right now the test for staticCache doesn't use the compress middleware.

@tj Owner
tj added a note

nvm i thought these were for negotiating via Vary at a glance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ it('should set X-Cache to MISS when missed', function(done){
+ app.request()
+ .set('Accept-Encoding', 'gzip')
+ .get('/todo.txt')
+ .expect('X-Cache', 'MISS', done);
+ })
+
+ it('should set X-Cache to HIT when hit', function(done){
+ app.request()
+ .set('Accept-Encoding', 'gzip')
+ .get('/todo.txt')
+ .expect('X-Cache', 'HIT', done);
+ })
+
+ it('should gzip files when asked', function(done){
+ app.request()
+ .get('/todo.txt')
+ .set('Accept-Encoding', 'gzip')
+ .end(function(res){
+ res.body.should.not.equal('- groceries');
+ done();
+ });
+ })
+
+ it('should not gzip files when not asked', function(done){
+ app.request()
+ .get('/todo.txt')
+ .expect('- groceries', done);
+ })
+
+ it('should set Vary', function(done){
+ app.request()
+ .get('/todo.txt')
+ .set('Accept-Encoding', 'gzip')
+ .expect('Vary', 'Accept-Encoding', done);
+ })
+
+ it('should set Vary at all times', function(done){
+ app.request()
+ .get('/todo.txt')
+ .expect('Vary', 'Accept-Encoding', done);
+ })
+
+ it('should set Content-Encoding', function(done){
+ app.request()
+ .get('/todo.txt')
+ .set('Accept-Encoding', 'gzip')
+ .expect('Content-Encoding', 'gzip', done);
+ })
+});
Something went wrong with that request. Please try again.