zlib: check callbacks are functions #2596
Conversation
| @@ -209,7 +209,8 @@ function zlibBuffer(engine, buffer, callback) { | |||
| function onError(err) { | |||
| engine.removeListener('end', onEnd); | |||
| engine.removeListener('readable', flow); | |||
| callback(err); | |||
| if (util.isFunction(callback)) | |||
There was a problem hiding this comment.
util.isFunction is deprecated, please use a typeof comparison
|
Hmmm, I didn't look at the original PR in any great detail when I tagged it. When the callback isn't a function isn't this a no-op? |
| @@ -0,0 +1,9 @@ | |||
| 'use strict'; | |||
| var common = require('../common'); | |||
There was a problem hiding this comment.
Can you make these three lines use const instead of var.
|
Are you asking if it should be a no-op, or if it is now? I don't know the code too well but it looks like it will still do all the deflate but just not try to call a callback. I'm not actually sure why that's useful though tbh. |
Yeah, that's what I'm saying, I'm pretty sure you can't get data out of it without a callback, so I'm not sure this is actually useful. :/ |
|
Ok it seems like we agree. I figured this was useful for some reason I don't know about. Happy to close if it's not needed. |
|
Closing. Not providing a callback here is effectively a bug in user code. @mtharrison thanks for the contribution, even if this ended up not being useful. Lets us check another issue off! :) |
|
No worries :) I might have a go at another one of those. |
This PR attempts to resolve issue #2397
It's based off the PR at: nodejs/node-v0.x-archive#6525
I have added tests too. This is my first contribution so please let me know if I missed something out.