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

Corrects bug with using compress and staticCache together. #458

Closed
wants to merge 2 commits into
base: master
from
Jump to file or symbol
Failed to load files and symbols.
+68 −1
Diff settings

Always

Just for now

Next

Fixed conflict with compress and staticCache that was preventing cach…

…ed items from being compressed
  • Loading branch information...
ryanrolds committed Jan 15, 2012
commit c6af858691414f3b6f911412302597dce61ac638
@@ -147,8 +147,14 @@ 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]);
});

This comment has been minimized.

@tj

tj Jan 15, 2012

Member

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

@tj

tj Jan 15, 2012

Member

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

This comment has been minimized.

@ryanrolds

ryanrolds Jan 15, 2012

Contributor

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.

@ryanrolds

ryanrolds Jan 15, 2012

Contributor

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.

This comment has been minimized.

@tj

tj Jan 15, 2012

Member

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

@tj

tj Jan 15, 2012

Member

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

This comment has been minimized.

@ryanrolds

ryanrolds Jan 15, 2012

Contributor

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.

@ryanrolds

ryanrolds Jan 15, 2012

Contributor

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.

This comment has been minimized.

@tj

tj Jan 15, 2012

Member

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

@tj

tj Jan 15, 2012

Member

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

res.writeHead(200);

This comment has been minimized.

@tj

tj Jan 15, 2012

Member

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

@tj

tj Jan 15, 2012

Member

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

This comment has been minimized.

@ryanrolds

ryanrolds Jan 15, 2012

Contributor

Good point. I will take care of that.

@ryanrolds

ryanrolds Jan 15, 2012

Contributor

Good point. I will take care of that.

This comment has been minimized.

@ryanrolds

ryanrolds Jan 15, 2012

Contributor

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

@ryanrolds

ryanrolds Jan 15, 2012

Contributor

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

// backpressure
function write(i) {
var buf = hit[i];
@@ -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(){

This comment has been minimized.

@tj

tj Jan 15, 2012

Member

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

@tj

tj Jan 15, 2012

Member

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

This comment has been minimized.

@ryanrolds

ryanrolds Jan 15, 2012

Contributor

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

@ryanrolds

ryanrolds Jan 15, 2012

Contributor

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

This comment has been minimized.

@tj

tj Jan 15, 2012

Member

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

@tj

tj Jan 15, 2012

Member

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

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);
})
});
ProTip! Use n and p to navigate between commits in a pull request.