Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: callbackify resulting function should have one more argument #72

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: callbackify resulting function should have one more argument #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
87caee5
fix: callbackify resulting function should have one more argument
erossignon May 18, 2022
aed9474
fix callbackify function length should keep old behavior on old brows…
erossignon May 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions support/isFunctionLengthConfigurable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@

module.exports = function isFunctionLengthConfigurable(arg) {
return Object.getOwnPropertyDescriptor(function () { }, 'length').configurable;
}
Comment on lines +2 to +4
Copy link
Member

Choose a reason for hiding this comment

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

this already exists on npm - https://npmjs.com/functions-have-names in the functionsHaveConfigurableNames property. let's just use that.

8 changes: 8 additions & 0 deletions support/nodeJSVersion.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@

module.exports = function nodeJSVersion(arg) {
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn’t be needed at all; the feature detection is sufficient (and more reliable)

Copy link
Author

@erossignon erossignon May 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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; // 42

but 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.

Copy link
Author

Choose a reason for hiding this comment

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

@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 in util.js .

Let me know if there is anything else I can do .

Copy link
Member

Choose a reason for hiding this comment

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

this still seems an issue, since this file shouldn't exist


if (!process || !process.version || !process.version.match(/^v(\d+)\.(\d+)/)) {
return false;
}
return parseInt(process.version.split('.')[0].slice(1), 10);
}
22 changes: 22 additions & 0 deletions test/browser/callbackify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
var test = require('tape');
var callbackify = require('../../').callbackify;

var isFunctionLengthConfigurable = require('../../support/isFunctionLengthConfigurable');

if (typeof Promise === 'undefined') {
console.log('no global Promise found, skipping callbackify tests');
return;
Expand Down Expand Up @@ -167,3 +169,23 @@ test('util.callbackify non-function inputs throw', function (t) {
});
t.end();
});

test('util.callbackify resulting function should have one more argument', function (t) {

if (!isFunctionLengthConfigurable()) {
console.log("skipping this test as function.length is not configurable in the current javascript engine");
return;
}
// Test that resulting function should have one more argument
[
function() { },
function(a) { },
function(a, b) { }
].forEach(function (fct) {

var callbackified = callbackify(fct);
t.strictEqual(callbackified.length, fct.length + 1, "callbackified function should have one more argument");
});
t.end();
});

33 changes: 33 additions & 0 deletions test/node/callbackify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,39 @@ var common = require('./common');
var assert = require('assert');
var callbackify = require('../../').callbackify;
var execFile = require('child_process').execFile;
var nodeJSVersion = require('../../support/nodeJSVersion');
var isFunctionLengthConfigurable = require("../../support/isFunctionLengthConfigurable");

(function callbackify_resulting_function_should_have_one_more_argument() {

if (!isFunctionLengthConfigurable()) {
console.log("skipping this test as function.length is not configurable in the current javascript engine");
return;
}
var nodeVersion = nodeJSVersion();

console.log("Testing callbackify resulting function should have one more argument")
var original_callbackify = require('util').callbackify;
// Test that resulting function should have one more argument
[
function () { },
function (a) { },
function (a, b) { }
].forEach(function (fct) {

var node_callbackified = original_callbackify(fct);
var browser_callbackified = callbackify(fct);

if (nodeVersion >= 12 && node_callbackified.length !== fct.length + 1) {
// this behavior is only true with node 12 and above, where the bug was fixed
throw new Error("callbackified function should have one more argument");
}
if (browser_callbackified.length !== node_callbackified.length) {
console.log("browser_callbackified=", browser_callbackified.length, "node_callbackified=", node_callbackified.length)
throw new Error("callbackified function should have one more argument, like in node");
}
});
})();

if (typeof Promise === 'undefined') {
console.log('no global Promise found, skipping callbackify tests');
Expand Down
16 changes: 12 additions & 4 deletions util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,13 +703,21 @@ function callbackify(original) {
// In true node style we process the callback on `nextTick` with all the
// implications (stack, `uncaughtException`, `async_hooks`)
original.apply(this, args)
.then(function(ret) { process.nextTick(cb.bind(null, null, ret)) },
function(rej) { process.nextTick(callbackifyOnRejected.bind(null, rej, cb)) });
.then(function (ret) { process.nextTick(cb.bind(null, null, ret)) },
function (rej) { process.nextTick(callbackifyOnRejected.bind(null, rej, cb)) });
}

Object.setPrototypeOf(callbackified, Object.getPrototypeOf(original));
Object.defineProperties(callbackified,
getOwnPropertyDescriptors(original));
const desc = getOwnPropertyDescriptors(original);
erossignon marked this conversation as resolved.
Show resolved Hide resolved
var isFunctionLengthConfigurable = Object.getOwnPropertyDescriptor(callbackified, 'length').configurable;
Copy link
Member

Choose a reason for hiding this comment

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

this should be using the file above, or better functions-have-names


// versions of NodeJS <12.0 have a bug where the callback's `length` is not adjusted as in recent NodeJS version
// even though functionLength is configurable.
if (isFunctionLengthConfigurable) {
desc.length.value += 1;
}

Object.defineProperties(callbackified, desc);
erossignon marked this conversation as resolved.
Show resolved Hide resolved
erossignon marked this conversation as resolved.
Show resolved Hide resolved
return callbackified;
}
exports.callbackify = callbackify;