fix callbackify function length should keep old behavior on old browser engine

Author: erossignon

support/isFunctionLengthConfigurable.js

@@ -0,0 +1,4 @@
+
+module.exports = function isFunctionLengthConfigurable(arg) {
+    return Object.getOwnPropertyDescriptor(function () { }, 'length').configurable;
+}

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); 
+}

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() { },

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");
-    }
-  });
-})();

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;
 }