Discuss ways to avoid `new Function` in denodeify

[mikesamuel]

I'd like to lockdown new Function and eval in favor of vm and other code execution mechanisms that can be checked via require or import.

This module seems to use them to create .length preserving wrapper functions that bridge callback code and Promises.

promise/src/node-extensions.js

Line 47 in d980ed0

return Function(['Promise', 'fn'], body)(Promise, fn);

I think the semantics of denodeify could be implemented without runtime code generation using tricks like the below:

const { apply: rapply, get: rget } = Reflect;

const handlerForArgumentCount = [];

function denodeify(fn, argumentCount) {
  if (typeof fn !== 'function') {
    throw new TypeError(typeof fn);
  }
  if (argumentCount !== (argumentCount >>> 0)) {
    argumentCount = fn.length;
  }
  if (!handlerForArgumentCount[argumentCount]) {
    handlerForArgumentCount[argumentCount] = {
      get(target, prop, receiver) {
        if (prop === 'length') {
          return argumentCount;
        }
        return rget(target, prop, receiver);
      },
      apply(target, thisArgument, argumentsList) {
        return new Promise((rs, rj) => {
          const args = [];
          for (let i = 0; i < argumentCount; ++i) {
            args[i] = argumentsList[i];
          }
          args[argumentCount] = (err, res) => {
            if (err) {
              rj(err);
            } else {
              rs(res);
            }
          };
          const res = rapply(target, thisArgument, args);
          if (res && (typeof res === "object" || typeof res === "function")
              && typeof res.then === "function") {
            rs(res);
          }
        });
      },
    };
  }
  const handler = handlerForArgumentCount[argumentCount];
  return new Proxy(fn, handler);
}

Would you be interested in patches that avoid using new Function so can run on runtimes that turn that off?

I notice you check compatibility with

promise/.travis.yml

Lines 4 to 7 in d980ed0

	node_js:
	- "4"
	- "6"
	- "8"

How tolerant is this project of feature detecting or loading different -impl.js files based on the availability of features like Proxy?

What would you need to see in terms of performance measurement before making a decision?

[ForbesLindesay]

I'm not super keen to rely on Proxy, and from what I recall, the primary reason we use new Function is actually for performance. In the case where we know the number of arguments up front, we can generate more optimised code that does not need to copy the arguments pseudo array.

One option might be to add a few prebuilt functions for 0, 1, 2, 3 and 4 arg functions. We could then avoid the runtime code generation until faced with a function that takes a large number of arguments.

In terms of performance, I'd want to see us doing the same or better in all the bluebird benchmarks https://github.com/petkaantonov/bluebird/tree/master/benchmark

In terms of having multiple versions, we so have a version for "domains" support and a version that uses "setImmediate" directly instead of asap. These are generated using the build.js script. We could add another output for content-security-policy compliance, which could either use your Proxy approach, or just skip enforcing the length of returned functions.