Skip to content

Commit

Permalink
remove awful res.end/res.write noop patch
Browse files Browse the repository at this point in the history
this removes the res.end/res.write to noop after request end patch,
which is unnecessary and is really weird, because it will stop errors
from triggerring that would normally trigger when this middleware
is not being used.

fixes #8
  • Loading branch information
dougwilson committed Jun 3, 2014
1 parent 73c50a9 commit 1412524
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 5 deletions.
5 changes: 0 additions & 5 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,6 @@ module.exports = function compression(options) {
, compress = true
, stream;

// see #8
req.on('close', function(){
res.write = res.end = function(){};
});

// flush is noop by default
res.flush = noop;

Expand Down
31 changes: 31 additions & 0 deletions test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,37 @@ describe('compress()', function(){
.end()
})

it('should not error when client aborts', function(done){
var closed = false
var resp
var server = createServer({ threshold: 0 }, function (req, res) {
resp = res
req.on('close', function(){ closed = true })
res.setHeader('Content-Type', 'text/plain')
res.setHeader('Content-Length', '2048')
res.write(new Buffer(1024))
res.flush()
})

request(server)
.get('/')
.set('Accept-Encoding', 'gzip')
.request()
.on('response', function (res) {
var req = this
res.headers['content-encoding'].should.equal('gzip')
res.once('data', function(){
req.abort()
setTimeout(function(){
closed.should.be.true
resp.end(new Buffer(1024))
done()
}, 10)
})
})
.end()
})

describe('threshold', function(){
it('should not compress responses below the threshold size', function(done){
var server = createServer({ threshold: '1kb' }, function (req, res) {
Expand Down

3 comments on commit 1412524

@jonathanong
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why these were added to the middleware in the first place

@dougwilson
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the history of connect. It's because people provide patches and the maintainers sometimes just blindly accept things... :D

@dougwilson
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch was provided for the same reason why someone might supply a patch to node.js to not get the Can't send headers after send error... because they were doing something in error, but just wanted to make the error not happen, lol.

Please sign in to comment.