Use defineProperty() to avoid eval() and new Function() while preserving arity

[agopshi]

Any thoughts on using defineProperty(deprecatedfn, 'length', {value: fn.length, ...}) to preserve arity with a regular function (and thus avoiding eval() and new 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.

[dougwilson]

Huh, interesting! I never realized you could define the function length. If this passes of course I'll accept it 🤣

[agopshi]

Hm, based on the CI builds, it looks like this works in Node.js 3.3+ and fails on 2.5 and earlier with TypeError: Cannot redefine property: length. Node.js 2.5 is ancient, but if you must support it then this might be a no-go.

[dougwilson]

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?

[agopshi]

That's definitely one idea. Another idea I'd like to try is delete deprecatedfn.length before calling defineProperty() - maybe it's the redefinition that's causing the problem.

[agopshi]

OK, deleting it first doesn't work either. Looks like our best bet is to either check process.version manually or wrap the defineProperty() attempt in a try-catch and fall back to the new Function() approach. I think the try-catch would be less fragile.

[dougwilson]

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 lib/compact while it would be at 100% for index.js across every Node.js version.

[agopshi]

Sweet, looks like the CI builds are happy now (I also tested locally with nvm). I won't be able to make the lib/compat change this week, but I can take a look next week if you don't get a chance to do it.

[dougwilson]

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.

[agopshi]

Not sure if you got a chance to take a look, but I went ahead and moved the logic into lib/compat with some simple detection logic. Feel free to change it up if you'd rather structure it differently.

[agopshi]

@dougwilson Friendly ping.

[david-fong]

Hello! What's the status on this PR?

I recently discovered it when trying to use nodejs's --disallow-code-generation-from-strings. That flag is a nice security feature kind of like the content-security-policy header. I can't use the flag right now because some of my dependencies use depd (body-parser, http-errors, send, and express).