Allow nfbind bound function to handle missing args #753
Conversation
The function bound by nfbind will behave properly when invoked with fewer args than expected, because we pad out the missing args before appending the callback.
| // Pad an array value with nulls to the specified length. Longer arrays are left alone. | ||
| function padArrayTo(arry,len) { | ||
| if ( arry.length < len ) { | ||
| arry = array_concat(arry, new Array(len - arry.length)); |
There was a problem hiding this comment.
This can be accomplished without allocations array.length = Math.min(array.length, length)
|
First glance, this looks like a legitimate improvement to me. cc @domenic for sanity check. Please consider making some concessions for the prevailing style. Spaces around operators, fully-spelled variable names (array over arry, length over len). |
|
I will change the variable names and look for any place I have squished operators. |
| @@ -345,6 +346,14 @@ function isObject(value) { | |||
| return value === Object(value); | |||
| } | |||
|
|
|||
| // Pad an array value with nulls to the specified length. Longer arrays are left alone. | |||
There was a problem hiding this comment.
This doesn't do what it says. It pads the array with holes, not nulls.
There was a problem hiding this comment.
Oops, you are right; I will change the comment.
|
This is a pretty major change in semantics and as such should only go into 2.0. I am neutral on the functionality; I can see how it might make sense to test fn.length to find the last argument, but in general I think that's fragile and am not sure relying on it is a good idea. |
|
Just to be clear, what currently happens is that if you omit a trailing argument, the call breaks with a TypeError, because the handler gets moved to an earlier argument position and is thus not available when called. Is it truly the case that this is intentional semantics ? |
|
Right, those are intentional semantics currently, because they are the simplest thing that can be done without any magic guessing that might e.g. hide bugs. Magic guessing is OK to add, but it's a major semantic change and deserves 2.0 only. fn.length is not incompatible between implementations, but it's just not always useful. For example, many core functions in the Node.js standard library have fn.lengths that mismatch their actual call signature, since they switch on |
|
Ah, yes I can see how that might cause problems. I leave it in your Denise Draper On Sun, Jan 3, 2016, at 04:39 PM, Domenic Denicola wrote:
Links: |
|
I’m compelled to leave the library as it stands. Undesirable as the behavior is in some cases, it is less surprising in the majority. Explicitly filling missing arguments at the call site seems safer than implicitly filling them in the library. Regardless, thank you for contributing. |
A common coding habit in javascript is to omit trailing arguments whose value is null. Currently a function created via nfbind will not behave properly if used this way, This patch allows the function to behave properly, by padding out any missing arguments with null values.