Skip to content

Adding callback to res.write and res.end for streaming support #80

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.

Sign up for GitHub

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

Closed
wants to merge 20 commits into from
Closed

Adding callback to res.write and res.end for streaming support #80

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
ed51c3a
Update index.js
May 8, 2016
f9c95f9
Update index.js
May 8, 2016
c4ba4af
Adding tests.
jpodwys May 23, 2016
bd60b96
Updating docs.
jpodwys May 23, 2016
5cd1fe2
no message
jpodwys May 23, 2016
2b85837
no message
jpodwys May 23, 2016
31792c1
Testing with and without compression.
jpodwys May 23, 2016
bda469c
Reverting readme change and ensuring callbacks are called in the orde…
jpodwys May 23, 2016
5f122e8
Adding error handling.
jpodwys May 23, 2016
eb1bad8
Fixing lint errors.
jpodwys May 23, 2016
ac8914b
Adding test with no streaming.
jpodwys May 24, 2016
866775d
no message
jpodwys May 24, 2016
c1f3aab
Attempting to handle pre-11 node versions.
jpodwys May 24, 2016
b0055d2
no message
jpodwys May 24, 2016
3ab2f0d
Handling node 0.8 and 0.10.
jpodwys May 24, 2016
79c6b75
Typo.
jpodwys May 24, 2016
0f617c7
Checking OutgoingMessage.prototype.
jpodwys May 24, 2016
4b50f90
Making res.flush synchronous.
jpodwys May 28, 2016
2e995d0
no message
jpodwys May 28, 2016
74fdf7b
no message
jpodwys May 28, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,10 +205,10 @@ app.get('/events', function (req, res) {

// send a ping approx every 2 seconds
var timer = setInterval(function () {
res.write('data: ping\n\n')

// !!! this is the important part
res.flush()
res.write('data: ping\n\n', function(){
// !!! this is the important part
res.flush()
})
}, 2000)

res.on('close', function () {
Expand Down
14 changes: 7 additions & 7 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ function compression (options) {

// proxy

res.write = function write (chunk, encoding) {
res.write = function write (chunk, encoding, cb) {
if (ended) {
return false
}
Expand All @@ -84,11 +84,11 @@ function compression (options) {
}

return stream
? stream.write(new Buffer(chunk, encoding))
: _write.call(this, chunk, encoding)
? stream.write(new Buffer(chunk, encoding), cb)
: _write.call(this, chunk, encoding, cb)
}

res.end = function end (chunk, encoding) {
res.end = function end (chunk, encoding, cb) {
if (ended) {
return false
}
Expand All @@ -103,16 +103,16 @@ function compression (options) {
}

if (!stream) {
return _end.call(this, chunk, encoding)
return _end.call(this, chunk, encoding, cb)
}

// mark ended
ended = true

// write Buffer for Node.js 0.8
return chunk
? stream.end(new Buffer(chunk, encoding))
: stream.end()
? stream.end(new Buffer(chunk, encoding), cb)
: stream.end(null, cb)
}

res.on = function on (type, listener) {
Expand Down
29 changes: 29 additions & 0 deletions test/compression.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,35 @@ describe('compression()', function () {
.end()
})
})

describe('when callbacks are used', function () {
it('should call the passed callbacks', function(done){
var callbacks = 0;
var server = createServer({ threshold: '1kb' }, function (req, res) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typically you should only pass in the options that are relevant to the test. Here you are passing in threshold: '1kb', which doesn't seem to be necessary for the test (and is even the default setting). Can this be removed? If not, what is the purpose for specifying it explicitly?

res.setHeader('Content-Type', 'text/plain')
res.write('Hello', null, function(){
callbacks++
res.flush()
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the test is labeled "when callbacks are used should call the passed callbacks", it doesn't sound like the test should be testing .flush() functionality. Is there a reason to call .flush() in theses callbacks? If it is testing something, should that be a different test, or can it be reflected in the name?

Copy link
Author

@jpodwys jpodwys May 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Good point, I'll remove it. I was simply including .flush() for completeness since nothing makes it to the client without that call.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. So perhaps that would call for two different tests? Testing the callback when not flushed vs testing with flushed? DO you think that would be a meaningful test?

});
res.write(' World', null, function(){
callbacks++
res.flush()
});
res.end(null, null, function(){
callbacks++
});
})

request(server)
.get('/')
.set('Accept-Encoding', 'gzip')
.expect('Content-Encoding', 'gzip')
.end(function(){
Copy link
Contributor

Choose a reason for hiding this comment

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

The first argument to this callback is an err and the code here is not handling the error. Please add error handling.

Copy link
Contributor

Choose a reason for hiding this comment

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

The first argument to this callback is an err and the code here is not handling the error. Please add error handling.

assert.equal(callbacks, 3)
done();
});
})
})
})

function createServer (opts, fn) {
Expand Down