From 87caee5bc82c40b7114a476273ccbf0c7861c3b8 Mon Sep 17 00:00:00 2001 From: "Etienne Rossignon (Sterfive)" Date: Wed, 18 May 2022 08:54:47 +0200 Subject: [PATCH 1/2] fix: callbackify resulting function should have one more argument --- test/browser/callbackify.js | 15 +++++++++++++++ test/node/callbackify.js | 26 ++++++++++++++++++++++++++ util.js | 5 +++-- 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/test/browser/callbackify.js b/test/browser/callbackify.js index ea05089..0f0ac26 100644 --- a/test/browser/callbackify.js +++ b/test/browser/callbackify.js @@ -167,3 +167,18 @@ test('util.callbackify non-function inputs throw', function (t) { }); t.end(); }); + +test('util.callbackify resulting function should have one more argument', function (t) { + // 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(); +}); + diff --git a/test/node/callbackify.js b/test/node/callbackify.js index 8da100a..9603c10 100644 --- a/test/node/callbackify.js +++ b/test/node/callbackify.js @@ -193,3 +193,29 @@ if (false) { if (require('is-async-supported')()) { require('./callbackify-async'); } + +(function callbackify_resulting_function_should_have_one_more_argument() { + + var nodeJSVersion = parseInt(process.version.substring(1,3),10); + + 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 (nodeJSVersion >= 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) { + throw new Error("callbackified function should have one more argument, like in node"); + } + }); +})(); diff --git a/util.js b/util.js index 6eea657..933c1e4 100644 --- a/util.js +++ b/util.js @@ -708,8 +708,9 @@ function callbackify(original) { } Object.setPrototypeOf(callbackified, Object.getPrototypeOf(original)); - Object.defineProperties(callbackified, - getOwnPropertyDescriptors(original)); + const desc = getOwnPropertyDescriptors(original); + desc.length.value += 1; + Object.defineProperties(callbackified, desc); return callbackified; } exports.callbackify = callbackify; From aed9474cc8fbc8b8167933e11fe85478960ccc18 Mon Sep 17 00:00:00 2001 From: "Etienne Rossignon (Sterfive)" Date: Thu, 19 May 2022 08:21:20 +0200 Subject: [PATCH 2/2] fix callbackify function length should keep old behavior on old browser engine --- support/isFunctionLengthConfigurable.js | 4 ++ support/nodeJSVersion.js | 8 ++++ test/browser/callbackify.js | 7 +++ test/node/callbackify.js | 59 ++++++++++++++----------- util.js | 13 ++++-- 5 files changed, 62 insertions(+), 29 deletions(-) create mode 100644 support/isFunctionLengthConfigurable.js create mode 100644 support/nodeJSVersion.js diff --git a/support/isFunctionLengthConfigurable.js b/support/isFunctionLengthConfigurable.js new file mode 100644 index 0000000..9c8d771 --- /dev/null +++ b/support/isFunctionLengthConfigurable.js @@ -0,0 +1,4 @@ + +module.exports = function isFunctionLengthConfigurable(arg) { + return Object.getOwnPropertyDescriptor(function () { }, 'length').configurable; +} \ No newline at end of file diff --git a/support/nodeJSVersion.js b/support/nodeJSVersion.js new file mode 100644 index 0000000..b00ce7a --- /dev/null +++ b/support/nodeJSVersion.js @@ -0,0 +1,8 @@ + +module.exports = function nodeJSVersion(arg) { + + if (!process || !process.version || !process.version.match(/^v(\d+)\.(\d+)/)) { + return false; + } + return parseInt(process.version.split('.')[0].slice(1), 10); +} diff --git a/test/browser/callbackify.js b/test/browser/callbackify.js index 0f0ac26..b6f32cc 100644 --- a/test/browser/callbackify.js +++ b/test/browser/callbackify.js @@ -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; @@ -169,6 +171,11 @@ test('util.callbackify non-function inputs throw', function (t) { }); 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() { }, diff --git a/test/node/callbackify.js b/test/node/callbackify.js index 9603c10..c20a3b1 100644 --- a/test/node/callbackify.js +++ b/test/node/callbackify.js @@ -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'); @@ -193,29 +226,3 @@ if (false) { if (require('is-async-supported')()) { require('./callbackify-async'); } - -(function callbackify_resulting_function_should_have_one_more_argument() { - - var nodeJSVersion = parseInt(process.version.substring(1,3),10); - - 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 (nodeJSVersion >= 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) { - throw new Error("callbackified function should have one more argument, like in node"); - } - }); -})(); diff --git a/util.js b/util.js index 933c1e4..8d5442a 100644 --- a/util.js +++ b/util.js @@ -703,13 +703,20 @@ 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)); const desc = getOwnPropertyDescriptors(original); - desc.length.value += 1; + var isFunctionLengthConfigurable = Object.getOwnPropertyDescriptor(callbackified, 'length').configurable; + + // 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); return callbackified; }