Respect --no-deprecation and process.noDeprecation

[jaydenseric]

It took me ages to realise why I couldn't silence some deprecation warnings using process.noDeprecation - The deprecation was coming from depd instead of regular Node.js util and apparently the standard flag is not respected.

[jaydenseric]

Also, regular Node.js deprecation warnings emit the warning event on process, something this library should probably do too?

[dougwilson]

This library should be respecting the command line argument. See the readme:

Providing the argument --no-deprecation to the node executable will suppress all deprecations (only available in Node.js 0.8 or higher).

This is also validated in our test suite. Can you provide the following so I can see what is happening?

(1) Node.js version
(2) version of this module
(3) example code to run
(4) instructions on how to run to reproduce the issue.

Thank you for the report!

[jaydenseric]

1. Node.js v10.11.0
2. depd@1.1.2

This works:

// test.js
const deprecate = require('depd')('my-module')
deprecate('Testing…')

node --no-deprecation test

While this does not (but should):

// test.js
const deprecate = require('depd')('my-module')
process.noDeprecation = true
deprecate('Testing…')

node test

[dougwilson]

Thanks! Sorry, I guess you were saying two separate things, the --no-deprecation command line argument and dynamically modifying process.noDeprecation at runtime. You can modify process.noDeprecation at runtime, as long as you do so before initializing the deprecation for the given module. Ideally this is not modified at runtime, which is similar to the caveats about modifying process.noDeprecation at runtime for Node.js's own util.deprecate as well, there the timing as which you set it will depend on if certain things are suppressed or not.

This module doesn't document or test the effect of altering process.noDeprecation at runtime, though. If you think the behavior should be different, you're welcome to make a pull request and I can consider it for the next major revision :) !

[jaydenseric]

This should be considered a bug rather than an enhancement.

If you are a consumer of a package that internally uses util.deprecate, you can temporarily disable deprecation warnings with process.noDeprecation, run a particular deprecated function, then re-enable deprecation warnings.

If you are a consumer of a package that interaly uses depd, process.noDeprecation has no effect at runtime, since you require the deprecated function before you use it.

[dougwilson]

I'm not sure I understand. Why would changing that property affect runtime? The documentation in Node.js is as follows:

https://nodejs.org/api/process.html#process_process_nodeprecation

The process.noDeprecation property indicates whether the --no-deprecation flag is set on the current Node.js process.

Forgive me if I'm wrong, but I don't think command like flags can be changed on a process after it's been started, no? I had intentionally implemented it as it is today, which is why it's not a bug, based on the description from the Node.js documentation. It's also of course consistent in that altering process.env.NO_DEPRECATION env var is allowed at the same points that process.noDeprecation is allowed, which makes it easier to reason about the effects of these different global flags.

The deprecation mechanism as you know it in Node.js today didn't exist when the 1.0 of this module was created, so there was nothing in Node.js to copy. If this is under an improvement to bring it more in line with the built-in Node.js implementation, it seems like a new major version to me (and also perhaps just use the built-in Node.js mechanisms now that Node.js has them included?).

I'm open to making changes, as I said above 👍

[jaydenseric]

This makes me wonder if runtime changes to process.throwDeprecation are not respected either. Here is an example of its usefulness for testing deprecation warnings: https://github.com/jaydenseric/graphql-upload/blob/e2574456491445f24adc4e4f40ec3f3fdad01d36/src/test.mjs#L1468

[jaydenseric]

You can definitely control Node.js process.noDeprecation and process.throwDeprecation at runtime. If you try modifying it at runtime you will see that it works.

[dougwilson]

Well, this module doesn't even check process.throwDeprecation and so it doesn't have any affect on this module.

You can definitely control Node.js process.noDeprecation and process.throwDeprecation at runtime. If you try modifying it at runtime you will see that it works.

I'm aware, of course you can. I was explaining why I made the decision of the current implementation I did, and also never mentioned anything about being able to redefine process.noDeprecation in the README. There was no Node.js deprecation framework stuff that there is today to reference when this module was created.

[dougwilson]

So it seems the conversations has kind of dropped of, with my response having no follow up after waiting 15 days. If you like, you're welcome to make some pull requests, either to enhance the documentation or to add functionality to this module.

One thing I noticed from re-reading through the conversation above and your code link is that of course another difference between this module and the Node.js core deprecation is that deprecations are not once-per-process like Node.js, but instead this module is meant for long-running processes and emits them once-per-unique-call-site. This if your code calls the same deprecated thing in 5 different places, your test suite will see 5 different deprecation messages for that thing with this module vs one 1 deprecation message using the Node.js core module.

Your code example using throwDeprecation is one way of course to test for deprecations in your test suite, while this module instead the method is to add an event listener for the event 'deprecation' on the process object. This (imo from designing it this way) has the benefit of not leaving code that doesn't expect an exception from entering a bad state for the rest of your test suite.

But again, ultimately this module was made to implement something different from what the Node.js util.deprecate did at the time of the original publishing. I tied into a few global states from core, like the --no-deprecation command line argument for usefulness that was available at the time.

[freewil]

Hm, so honestly didn't read through this entire thread here, but it seems one major difference between this module and util.deprecate is that util.deprecate will only emit a warning once per process run.

I'd be happy to open a PR to do so, but I think a one line change would fix the biggest concern here, which is that this module emits a non-standard deprecation event instead of a standard warning event that util.deprecate would use. I'd suggest using process.emitWarning instead.