Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

fix(zlib) add check is callback is function #6525

Closed
wants to merge 1 commit into from
Closed

fix(zlib) add check is callback is function #6525

wants to merge 1 commit into from

Conversation

coderaiser
Copy link

Add check is callback is function to prevent next error.

zlib.js:160
    callback(err);
    ^
TypeError: undefined is not a function
    at Unzip.onError (zlib.js:160:5)
    at Unzip.EventEmitter.emit (events.js:95:17)
    at Zlib._binding.onerror (zlib.js:298:10)

I have one suggestion. Could be used construction like this:

function exec (callback, params) {
    if (util.isFunction(callback))
       callback(params);
}

Like it's done here. So there is would be less code.

Add check is callback is function to prevent next error.
zlib.js:160
    callback(err);
    ^
TypeError: undefined is not a function
    at Unzip.onError (zlib.js:160:5)
    at Unzip.EventEmitter.emit (events.js:95:17)
    at Zlib._binding.onerror (zlib.js:298:10)
@Nodejs-Jenkins
Copy link

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

Commit coderaiser/node@d8ed749bea4b831a73586643b4905d94ac80c9ef has the following error(s):

  • Commit message must indicate the subsystem this commit changes

The following commiters were not found in the CLA:

  • coderaiser

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

@indutny
Copy link
Member

indutny commented Nov 15, 2013

Hey!

Thanks for contributing, although I can't accept this change, I have a suggestion for you. Please take a look at https://github.com/joyent/node/blob/master/lib/fs.js#L91, ideally this could could be moved to lib/util.js and exported prefixed with _ and reused in both fs.js and zlib.js (probably few other places later).

Does it sounds interesting to you? Or would it be better for me to open an issue and work on it myself instead?

Please let me know what you think about it.

@coderaiser
Copy link
Author

I think that is good idea, but would be better if you will wok on it by yourself. I'm not so good at node sources for now.
Anyway reusing code is always good idea so live link please here when you done, it is interesting to me look to result :).

@bnoordhuis
Copy link
Member

Not sure if a single check is worth factoring out into a separate method.

FWIW, the change itself looks acceptable to me but please see CONTRIBUTING.md for hints on what the commit log and tests cases should look like.

@indutny
Copy link
Member

indutny commented Nov 18, 2013

@bnoordhuis we have a lot of such places in core, I think we will generally benefit from sharing this function between them.

@@ -185,13 +185,18 @@ function zlibBuffer(engine, buffer, callback) {
function onError(err) {
engine.removeListener('end', onEnd);
engine.removeListener('readable', flow);
callback(err);
if (util.isFunction(callback)) {
Copy link
Member

Choose a reason for hiding this comment

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

Please omit braces here.

@indutny
Copy link
Member

indutny commented Feb 18, 2014

Let's land it ;)

@jasnell
Copy link
Member

jasnell commented Aug 13, 2015

Well.. never seems to have landed :-( @indutny ... any other thoughts on this?

@coderaiser ... if you're still interested in getting this landed, unless @joyent/node-tsc feel that this is something we should land in v0.12, it may be best to close this here and open it against master in http://github.com/nodejs/node (which is now the active development stream)

@indutny
Copy link
Member

indutny commented Aug 13, 2015

@jasnell after a year I could only say that it needs a test. Other than that - everything is still looking good (except those braces)

@jasnell
Copy link
Member

jasnell commented Aug 16, 2015

Closing this here as its not going to be able to land in master. If this is going to land, it needs to be a new PR against http://github.com/nodejs/node. I've opened an issue to track.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants