fix: callbackify resulting function should have one more argument #72
Conversation
|
In which node version is this the case? Bear in mind that the current node-util largely matches node 0.12. |
erossignon
force-pushed
the
BugFix/Callbackified_Function_should_have_one_more_argument
branch
from
f2409b2 to
87caee5
Compare
| @@ -0,0 +1,8 @@ | |||
|
|
|||
| module.exports = function nodeJSVersion(arg) { | |||
There was a problem hiding this comment.
This shouldn’t be needed at all; the feature detection is sufficient (and more reliable)
There was a problem hiding this comment.
can you elaborate?
I found that detecting the nodeJSVersion was needed because, in versions 10 and 11 of NodeJS, the length is not increased although the function length is configurable; the fix was only introduced in version >=12. I needed a way to detect these 2 special cases in the unit test.
(see table above)
There was a problem hiding this comment.
I'm skeptical - in my experimentation, it works fine in node 10:
function f() {}
Object.getOwnPropertyDescriptor(f, 'length').configurable; // true
Object.defineProperty(f, 'length', { value: 42 });
f.length; // 42but then we should be feature-detecting this failure. we shouldn't be hardcoding specific versions of things, especially since any issue in specific node versions also matches issues in some Chrome versions.
There was a problem hiding this comment.
@ljharb , yes fct.length is configurable in node10 onward but the util.callbackify do not take advantage of this and keep the fct.length unchanged. the behavior has been fixed by the nodejs team from node12 onwards.
we shouldn't be hardcoding specific versions of things,
I have remove the code that check against a specific version of node inutil.js.
Let me know if there is anything else I can do .
There was a problem hiding this comment.
this still seems an issue, since this file shouldn't exist
erossignon
force-pushed
the
BugFix/Callbackified_Function_should_have_one_more_argument
branch
from
41b988e to
aed9474
Compare
|
Any updates on this? :) |
| module.exports = function isFunctionLengthConfigurable(arg) { | ||
| return Object.getOwnPropertyDescriptor(function () { }, 'length').configurable; | ||
| } |
There was a problem hiding this comment.
this already exists on npm - https://npmjs.com/functions-have-names in the functionsHaveConfigurableNames property. let's just use that.
| @@ -0,0 +1,8 @@ | |||
|
|
|||
| module.exports = function nodeJSVersion(arg) { | |||
There was a problem hiding this comment.
this still seems an issue, since this file shouldn't exist
| Object.defineProperties(callbackified, | ||
| getOwnPropertyDescriptors(original)); | ||
| const desc = getOwnPropertyDescriptors(original); | ||
| var isFunctionLengthConfigurable = Object.getOwnPropertyDescriptor(callbackified, 'length').configurable; |
There was a problem hiding this comment.
this should be using the file above, or better functions-have-names
Fixing a callbackify behavior that is different from the NodeJS implementation ( when NodeJS version >= 12.0)
In node, the callbackified function has a length that is one more than the length of the original function. For instance:
this is not the case in the current version (<=0.12.4) where
This PR provides the fix and the associated unit test.