-
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
Use defineProperty() to avoid eval() and new Function() while preserving arity #26
base: master
Are you sure you want to change the base?
Conversation
Huh, interesting! I never realized you could define the function length. If this passes of course I'll accept it 🤣 |
Hm, based on the CI builds, it looks like this works in Node.js 3.3+ and fails on 2.5 and earlier with |
Hm. The goal of this module is to support as many possible Node.js versions as possible, especially since a lot of the time you deprecate things in order to move a library forward, so deprecating usually ends up in the older versions of things (and thus running on older versions of Node.js). For example Express still support 0.10 and this module is heavily used in there. Express is working to drop that support, but it's certainly really useful to have a deprecation module to help move folks forward. Is it possible to have the code use the old way for older Node.js versions and the define way for newer versions transparently? |
That's definitely one idea. Another idea I'd like to try is |
OK, deleting it first doesn't work either. Looks like our best bet is to either check |
Yea, that makes a lot of sense to me, less fragile the better. Feature detection FTW. So we have to do this for a couple items (one was removed recently), so this sounds like the same thing. Compat code lives in https://github.com/dougwilson/nodejs-depd/tree/master/lib/compat where the index provides an export that depends on feature detection and then a file for the implementation. This organization helps with figuring out low code coverage between Node.js versions, since it's expected for the coverage to vary in |
Sweet, looks like the CI builds are happy now (I also tested locally with |
Awesome! And I appreciate letting me know, happy to spend some time on this this weekend if I can, so won't feel bad if I get it done before you can now 👍 . This is a very popular request, so very excited to get this in. |
Not sure if you got a chance to take a look, but I went ahead and moved the logic into |
@dougwilson Friendly ping. |
aa7785a
to
3b11d7f
Compare
Hello! What's the status on this PR? I recently discovered it when trying to use nodejs's |
Any thoughts on using
defineProperty(deprecatedfn, 'length', {value: fn.length, ...})
to preserve arity with a regular function (and thus avoidingeval()
andnew Function()
entirely)?Tests pass, and I haven't been able to break it manually. Curious if I've missed any situations where this wouldn't work.