-
Notifications
You must be signed in to change notification settings - Fork 87
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.
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
Drop eval usage #20
Comments
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) :) |
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 :) |
If you could tell me what the problem is if we remove eval, perhaps I could help us solve it |
You need to preserve the original function's arity. |
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)
} |
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. |
I am trying to use the |
There are workarounds like the |
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. |
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. |
Electron is basically Chromium + Node.js so it has the node's native |
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. |
So @steelbrain I was just looking into |
Thanks for looking into this. My modules weren't using a deprecated function so I never encountered that error, my modules were merely |
Gotcha. I'm still investigating this approach to using |
@dougwilson Have you considered something like this?
|
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. |
@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? |
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. |
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? |
In my case, it was happening in a patched version of Node.js that calls 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. :) |
eval
usage in this function disallows this to be used in Chrome Apps or Electron Apps like Atom PackagesThe usage seems simple straight forward, that eval could be replaced by a closure
The text was updated successfully, but these errors were encountered: