lib: consistent callback checking across project by MylesBorins · Pull Request #3539 · nodejs/node · GitHub

Throughout the codebase there are two patterns for checking if a
callback has been provided as an argument. This PR replaces all
instances to do explicit type checking.

Closes #3536

I had originally done this with a regex... and I am realizing now why you might want to check for callback... what if it doesn't exist :s

although if (typeof callback === 'function') will still evaluate false whether or not callback is defined.

Seem like the extra if statement here isn't neccessary

Likely semver-major, yeah?

LGTM. Just out of curiosity, what can break from this change that wouldn't already be a crash before?

This is an interesting one. Prior to the change a callback that isn't a function would throw. In this instance it would fail silently

Probably needs an else if (callback) throw … or something like that

@cjihrig Can't tell without checking more closely, but it seems extremely likely that things will fail differently. So, someone may have code wrapped in a try/catch that expects a TypeError but now gets something else entirely.

Which actually brings up a point: Should these be changed so that there's an else that says that if callback is truthy but not a function, throw a TypeError? And if so, why not just leave them the way they are?

EDIT: I guess one advantage is you could be more stringent and say that callback needs to be null, undefined, or a function and anything else is a TypeError whereas the current situation will let falsy values like 0 slip by.

That is REALLY pushing the line between a fix and a breaking change, especially since most of these are asynchronous.

I wouldn't be opposed to throwing when a truthy, non-function value is passed, but then we're definitely into breaking change territory.

@Trott just brought up the exact thing I was just thinking. With this change it is possible for things to fail silently.

That being said, is it the duty of node.js to do type checking on function calls?

@cjihrig I'm OK with removing the semver-major tag if that's what consensus is. One litmus test perhaps: Would we want to port this change to LTS? I'd argue "no" and would therefore favor semver-major in this case.

I'm still not really convinced that this is semver-major, but I'm fine with making it semver-major just for the sake of extra caution.

Question for ya'll... how do we avoid the two patterns from diverging in the future.

Would it be unreasonable to make a linting rule for this type of thing?

CI, in case there are any surprises: https://ci.nodejs.org/job/node-test-pull-request/623/

This is not necessary. The on function takes care of it already. I would say you can simply remove the if guard around on, once, and removeListener, as they check if the parameter passed to them are functions.

didn't mean to bury this but realized that there was a bug here.

Want to continue the conversation at --> https://github.com/nodejs/node/pull/3539/files#r43200588

I think semver-major is the safest bet on this one. This is exactly the kind of change that leads to "oh crap we broke the world" moments immediately after cutting a new release. Throwing on a truthy non-function is a very good idea, I think.

A linting rule on callback type checking would be an interesting approach. We really ought to be more consistent about it.

Throughout the codebase there are two pattern for checking if a callback has been provided as an argument. This PR replaces all instances to do explicit type checking. Closes #3536

continuing https://github.com/nodejs/node/pull/3539/files#r43079350

@thefourtheye so you are suggesting that this Type check shouldn't be done.

@Trott suggested otherwise in --> #3539 (comment)

@jasnell was for it as well --> #3539 (comment)

I'm for it if there is the typeof check. I think @thefourtheye is saying that you can discard the typeof check and the else block and just pass blindly to .on() because .on() will throw a TypeError if the callback is not a function. At least, that's my understanding. So I'm for either approach, really. Don't make no neither nor to me.

In other words, I think @thefourtheye is saying this would be fine:

if (callback) {
    this.on('timeout', callback);
}

Which throws a wrench in the whole "standardize on typeof checks" approach.

We need that if guard if and only if callback might be optional and therefore undefined. I didn't look at the code to see if that is actually possible.

Right... At least the event handler code doesn't need this if block as it is done anyway in on, once, {add,remove}listener functions

Any thoughts on if this is a change we want to adopt?

seems like maybe we need a util._checkIsCallbackArgument() or something that does the typeof and also will throw a TypeError on a truthy non-function

I'm down with this either way, the current truthy is a smell and is now inconsistent across lib/

I like the idea of a generic util used across the lib for consistency. Would it make sense for this to land first as an intermediary step to make sure too many things don't blow up, and then add the refactoring with the util afterwards?

Well, landing this first means that we go from callback is not a function errors to silently failing, adding in a truthy-non-function check would put us to a TypeError which is on par with the runtime callback is not a function error, so landing this first puts us further away in terms of compatibility.

I think it would be best to add a truthy-non-function check with TypeError before landing this, whether it's done with a shared util function or done inline. Anyone else want to weigh in on this?

I definitely agree with @rvagg
On Oct 28, 2015 4:32 PM, "Rod Vagg" notifications@github.com wrote:

Well, landing this first means that we go from callback is not a function
errors to silently failing, adding in a truthy-non-function check would put
us to a TypeError which is on par with the runtime callback is not a
function error, so landing this first puts us further away in terms of
compatibility.

I think it would be best to add a truthy-non-function check with
TypeError before landing this, whether it's done with a shared util
function or done inline. Anyone else want to weigh in on this?

—
Reply to this email directly or view it on GitHub
#3539 (comment).

Ok I did a first pass at implementing the util function and came across some weirdness while running the test suite. Turns out the expected behavior of some functions that take callbacks is to fail silently if a bad argument is given :(

socket.close is such an example

https://github.com/TheAlphaNerd/node/blob/null-check-drain-listener/test/parallel/test-dgram-close-is-not-callback.js#L13-L14

What do people think of being stricter in these instances... obviously semver-major, but is it desirable to change this much underlying logic?

So digging further into dgram Socket.close it would appear that the callback argument is optional. It is not documented that an incorrect argument will be ignored. I have not gone through all instances of an optional callback, but at the very least we can deduce that the narrative is inconsistent. This will definitely be breaking, but I personally see value in consistency.

anyone have thoughts on the above information?

Looks like all other dns functions perform a callback check except dns.lookupService (https://github.com/nodejs/node/blob/master/lib/dns.js#L185-L195). Makes it sense to include it in this PR?

I am closing this for now as it is not entirely obvious that this will be easy to land (with various ways to handle callbacks)

MylesBorins reviewed Oct 27, 2015
View reviewed changes

Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 27, 2015

MylesBorins reviewed Oct 27, 2015
View reviewed changes

thefourtheye reviewed Oct 27, 2015
View reviewed changes

lib: consistent callback checking across project

ce7e901

Throughout the codebase there are two pattern for checking if a callback has been provided as an argument. This PR replaces all instances to do explicit type checking. Closes #3536

MylesBorins reviewed Oct 27, 2015
View reviewed changes

MylesBorins mentioned this pull request Nov 1, 2015

http: check callback type of imcoming timeout #3618

Closed

MylesBorins mentioned this pull request Nov 3, 2015

zlib: only apply drain listener if given callback #3534

Closed

MylesBorins closed this Dec 14, 2015

lib: consistent callback checking across project by MylesBorins · Pull Request #3539 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: consistent callback checking across project - #3539

lib: consistent callback checking across project#3539MylesBorins wants to merge 1 commit intonodejs:masternodejs/node:masterfrom MylesBorins:callback-explicit-type-checkCopy head branch name to clipboard

Conversation

MylesBorins commented Oct 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Oct 27, 2015

Uh oh!

cjihrig commented Oct 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Oct 27, 2015

Uh oh!

cjihrig commented Oct 27, 2015

Uh oh!

MylesBorins commented Oct 27, 2015

Uh oh!

Trott commented Oct 27, 2015

Uh oh!

cjihrig commented Oct 27, 2015

Uh oh!

MylesBorins commented Oct 27, 2015

Uh oh!

Trott commented Oct 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Oct 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Oct 28, 2015

Uh oh!

rvagg commented Oct 28, 2015

Uh oh!

MylesBorins commented Oct 28, 2015

Uh oh!

rvagg commented Oct 28, 2015

Uh oh!

jasnell commented Oct 28, 2015

Uh oh!

MylesBorins commented Oct 29, 2015

Uh oh!

MylesBorins commented Oct 29, 2015

Uh oh!

MylesBorins commented Nov 3, 2015

Uh oh!

r-52 commented Nov 8, 2015

Uh oh!

MylesBorins commented Dec 14, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

lib: consistent callback checking across project#3539
MylesBorins wants to merge 1 commit into
nodejs:masterfrom
MylesBorins:callback-explicit-type-check