Drop eval usage

[steelbrain]

eval usage in this function disallows this to be used in Chrome Apps or Electron Apps like Atom Packages The usage seems simple straight forward, that eval could be replaced by a closure

[dougwilson]

Hi! Always feel free to make a pull request if you are able to remove the eval yet keep the function arity (I should have tests that validate if you break anything on accident) :)

[dougwilson]

As far as I know,it is not possible to preserve arity without eval, which is why I used it. I'm going to close the issue, since there is nothing I can do by myself. If you have an idea of how to approach this please let me know, or better yet, make a proposal via a pull request :)

[steelbrain]

If you could tell me what the problem is if we remove eval, perhaps I could help us solve it

[dougwilson]

You need to preserve the original function's arity.

[steelbrain]

I must be misunderstanding, doesn't this solve the problem? The only side effect we will have is a bit of performance hit, but it's a deprecated function, not recommended anyway so I think it's fair

function proxy() {
  console.log('this thing is deprecated')
  realFunction.apply(this, arguments)
}

[dougwilson]

Yes, you are misunderstanding the problem. You need to preserve the original function's arity, which your example does not (it changes the function arity to zero, no matter what the original arity was). This is a feature of this module and will not be removed, because there are APIs that check function length, and if that function is deprecated, it should not have a different length simply because it is deprecated. This would break APIs, which is the entire point of this module: not to break APIs. A very popular module that requires function arity to stay intact is "async", for example.

[steelbrain]

I am trying to use the express and send modules in an electron app and hitting this error, thought it could be fixed. I guess not

[steelbrain]

There are workarounds like the loophole module, but I thought to fix it the right way. Thank you for your cooperation :)

[dougwilson]

Regardless, I don't think I'm asking for you to do much: make a pull request with your proposed change. Do not remove or break any existing tests. Your PR should be accepted.

[dougwilson]

I'm not familiar with electron apps. Do those run in Node.js? If not, you should consider building with a tool chain that uses the browser version of this module, which does not use eval.

[steelbrain]

Electron is basically Chromium + Node.js so it has the node's native require, therefore instead of browser it uses the node version. It's some security feature of Chromium that it does not allow unsafe eval within apps. I am using the loophole to make this module work now and it's working so far

[dougwilson]

I see this loophole uses the vm module. I'll take a look into this yo see if it can be used instead of eval directly in this module.

Since this module has never had dependencies and supports back to Node.js 0.6 not sure how feasible making that change will be, though.

[dougwilson]

So @steelbrain I was just looking into loophole and I tried replacing the eval usage in this module with vm.runInThisContext, but that does not work (at least as a straight replacement) due to the error ReferenceError: log is not defined when the returned function is actually ran. It looks like this is because according to the Node.js documentation, the code does not have access to the local scope, only global, so that function has no access to anything in our codebase.

[steelbrain]

Thanks for looking into this. My modules weren't using a deprecated function so I never encountered that error, my modules were merely requireing express so it replaced all the evals invoked by top level code to vm.runInThisContext

[dougwilson]

Gotcha. I'm still investigating this approach to using vm or something else to replace eval when possible. I'll let you know what I find out :) If you can also help, that would be great to move it along, but otherwise I think this can possibly be solved in some way to remove eval but keep providing preserved arity on wrapped functions.

[LukeStebbing]

@dougwilson Have you considered something like this?

function wrapFunction(fn) {
  var wrapperFunctions = [
    function() {
      return fn.apply(this, arguments);
    },
    function(arg) {
      return fn.apply(this, arguments);
    },
    function(arg, arg) {
      return fn.apply(this, arguments);
    },
    // ... 9-arity or so should be high enough for almost everything.
  ];
  return fn.length < wrapperFunctions.length
    ? wrapperFunctions[fn.length]
    : wrapFunctionUsingEval(fn);
}

[dougwilson]

Hi @LukeStebbing there is a PR right now to that effect, but that does not solve the issue at hand here. The appearance of the "eval" function at all is the issue, not if it appears within the used code path. That wouldn't solve the issue is "eval" or "new Function" is still in the source code so electron will not run the code.

[LukeStebbing]

@dougwilson Interesting, I always assumed that CSP used v8::Context::AllowCodeGenerationFromStrings() under the hood, and I think that allows eval as long as it doesn't appear in any function that's actually invoked. I'm not completely sure about that though.

Do you think it'd be workable to support arity up to a fairly high value and then throw an error if attempting to deprecate too-high arity of a function?

[dougwilson]

I assume we can deprecate using the API for above some value and run a deprecation cycle within this module (the same users would use this module itself for) to give ample time to migrate to whatever the new API is, provide clear path to migrate and what users need to do instead and then a few months later release a new major version with the new behavior.

[dougwilson]

I'm not familiar with how CSP works, but also mot familiar with how electron apps work; just going what people are saying because I haven't been provided any working examples and instructions on how to reproduce whatever the issue is that is trying to be solved here, so I can't experiment with different solutions to thr problem in any way. Are you also having an issue with Electron App or something else?

[LukeStebbing]

In my case, it was happening in a patched version of Node.js that calls v8::Context::AllowCodeGenerationFromStrings(). That code isn't open source, so unfortunately I can't easily provide a repro, but the symptom is that the send module throws EvalError: Code generation from strings disallowed for this context when SendStream.prototype.etag is defined: https://github.com/pillarjs/send/blob/ff0d82ee71ad884966f062f75d5b2b2a520ddd59/index.js#L183

I can simply enable eval in this case, so it's not a blocker for me, but I figured I'd make a drive-by comment just in case it was helpful. :)