Comments (9)
I see there was a similar discussion about this eval
ed code in #16 where it's explained that the arg0, arg1, ...
stuff is to maintain the function's arity.
One option would be to use the (fairly reasonable) assumption that most functions have relatively low arity;
const wrappers = [
function (fn, log, deprecate, message, site) {
return function() {
log.call(deprecate, message, site)
return fn.apply(this, arguments)
}
},
function (fn, log, deprecate, message, site) {
return function(arg0) {
log.call(deprecate, message, site)
return fn.apply(this, arguments)
}
},
function (fn, log, deprecate, message, site) {
return function(arg0, arg1) {
log.call(deprecate, message, site)
return fn.apply(this, arguments)
}
},
function (fn, log, deprecate, message, site) {
return function(arg0, arg1, arg2) {
log.call(deprecate, message, site)
return fn.apply(this, arguments)
}
}
// (etc. until you think enough parameters are supported)
]
function fallback() {
throw new Error('unable to wrap function with so many arguments');
}
// later:
var deprecatedfn = (wrappers[fn.length] || fallback)(fn, log, this, message, site)
Which could be combined with the options of using new Function
if available (in that case fallback
should probably fail non-destructively)
I'm sure there are lots of ways to reduce the duplication; this is just intended as a quick summary of the basic idea.
from nodejs-depd.
Another possibility which may or may not work here;
Object.defineProperty(deprecatedfn, 'length', {value: fn.length})
This successfully changes the .length
property of the function, but I don't know if it does it realistically enough for your needs in this project (I'm not sure where the arity of the function matters so I don't know if this applies to it).
from nodejs-depd.
Hi, I too also would love to see eval gone.the current plan ia here: #33 (comment)
I created this module for Express which supports too far down, Node.js 0.10. The Express 5 won't, so that's why the idea here is to release this change in conjection with Express to change to this new function length method.
It seems you are indeed just seeing this though Express usage, so that should have you covered.
Please let me know if the length method does not work so we can make a different plan. Also it would be a good idea to add that command line to our test suite, and you're welcome to contibute that as well
from nodejs-depd.
I think all 3 current pull requests are keeping support for all versions of NodeJS though (because they all fall-back in one way or another to using eval
ed code), so I don't think this needs to be a breaking change at all? (all the PRs are passing the checks in the existing CI environments; the latest from me failed one but that appears to be a flake rather than a real issue, and I have no attachment to it in any case; any of the active PRs would fit my needs).
To clarify, the flag I'm talking about here doesn't do any static analysis, it just causes eval
or new Function
etc. to throw instead of compiling anything if they are actually called. So it's no problem to have eval
around as a fall-back path.
Happy to update the test suite to add this flag in one/all of the PRs. I assume it would just be another CI environment with a recent version of Node and the flag turned on.
from nodejs-depd.
@dougwilson I updated the CI scripts to run with and without eval
enabled in each environment that supports it (i.e. NodeJS >= 9).
I also had to make one of the tests explicitly ignore this error (it checks behaviour when used with eval
, so the test itself causes eval
to be called). It uses this.skip()
to mark the test as skipped if eval is disabled. I had to make a similar change to the browserify
tests, because they have the same problem (the tests themselves need eval
).
All of these tests are still run in every environment, because the tests are run with and without eval
enabled for each version.
You can see this change in my PR #42 but it can easily be applied to any of the other PRs. It's just a small change to the package.json and CI config; see 51f1ef8 and a3d3099
(not sure why Node 5.12 is consistently failing in Travis but it's not related to the changes here; some problem installing browserify)
from nodejs-depd.
Hi @davidje13 you are correct in that they support old versions; I did mention that in the comment I linked to. There are also people who do use static analyzers who want it gone too. There is no reason to keep two different implementations and it's a great opportunity to drop old Node.js versions from this module. That is why this module will just bump requirement to Node.js 4+ and just have the simple function length implementation.
from nodejs-depd.
Hi @davidje13 it looks like we commented at the same time. My comment above was in response to the comment above your most recent comment.
For your most recent comment, I just took a look at the tests, and it doesn't seem right to me. A user (or CI) should just need to call npm test
to run everything; I was thinking about having this switch similar to how the --no-deprecation
and similar command line Node.js switches are tested in the test suite for the --disallow-code-generation-from-strings
from nodejs-depd.
@dougwilson it should be possible to do that, but when I tried as my first-pass, I hit problems with the code coverage tool failing (it needs eval
itself).
However you want to reshuffle the npm
commands should be fine, as long as the code coverage doesn't include the eval
-less version. So for example there could be explicit test-eval
and test-noeval
, with test
just invoking both but coverage only using test-noeval
(and related changes to which commands are called in CI as well).
from nodejs-depd.
I updated it to use cross-env
for Windows support (i.e. appveyor). Turns out that package doesn't support Node 0.8.0, but if you're planning to drop support for that anyway, maybe it doesn't matter.
I also reshuffled the commands to give another example of how this could be done. Now npm test
will cause the suite to run both with and without eval allowed, which sounds like it's what you're looking for? (it means that the code coverage commands don't run test
any more; they run test-eval
instead)
from nodejs-depd.
Related Issues (20)
- Stack trace api differences when in strict mode causing TypeErrors HOT 25
- Fail gracefully in unsupported browsers HOT 15
- Drop eval usage HOT 21
- line info is not help HOT 8
- Cannot redefine property: callSiteToString HOT 2
- Call-site calculation does not fail gracefully when Stack information is unavailable HOT 14
- Call site calculation fails when importing an esm'ed package that imports sequelize internally HOT 14
- Respect --no-deprecation and process.noDeprecation HOT 17
- TypeError: eval is not a function HOT 8
- Why is this library overwriting Error type? HOT 5
- Turkish Language Problem HOT 2
- Incopatibile with --enable-source-maps node 12 option HOT 7
- (!) Use of eval is strongly discouraged, rollup HOT 1
- compatibility with Node.js' source-map implementation HOT 12
- please rewrite nodejs-depd with modern syntax HOT 1
- Use of eval() is strongly discouraged, as it poses security risks HOT 9
- Calling `process.cwd()` from index.js can be problematic HOT 4
- callSite.getFileName() is not Function HOT 5
- `callSite.getFileName` is not a function HOT 10
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from nodejs-depd.