Respect --no-deprecation and process.noDeprecation about nodejs-depd HOT 17 CLOSED

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

from nodejs-depd.

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!

from nodejs-depd.

1. Node.js v10.11.0
2. [email protected]

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

from nodejs-depd.

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 :) !

from nodejs-depd.

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.

from nodejs-depd.

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 👍

from nodejs-depd.

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

from nodejs-depd.

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.

from nodejs-depd.

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.

from nodejs-depd.

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.

from nodejs-depd.

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.

from nodejs-depd.

Hi @freewil . So the reason Node.js uses the warning event and not our event is because they were originally going to use the event this module uses, but in a completely different manor. I asked them in nodejs/node#4782 (comment) to not use the event name this module was using. Moving this module to use the same event name that Node.js core uses would have the same effect in the end.

from nodejs-depd.

Would changing this module to use process.emitWarning() be an issue? It would be a major-version change, but I think it would help standardize deprecation events. With util.deprecate emitting warning events that is kind of the de-facto standard.

This module seems to already conform to util.deprecate functionality in every other way (--trace-deprecation and --no-deprecation support), but I was surprised to find an app that had been instrumented to handle process warnings for deprecations and other warnings not logging express' deprecations because of the difference in event names, warning vs deprecation.

from nodejs-depd.

Hi @freewil I want this module to use the deprecation event, not the warning event. If there is any de-facto standard, it is this module, which existed for years emitting the deprecation event before Node.js emitted deprecations as an event. I think they are the ones in the wrong, in this case. I expressed such when they were building out the deprecation event feature into core.

Another issue here is that process.emitWarning() was not added to Node.js until the 6.x series, but this module supports down to the 0.8 series, which makes that API out of reach. And your example of Express supports down to 0.10, which even if this module were to dump support for all Node.js below 6.x (it won't), Express.js wouldn't then be able to use it (it would be a major change for Express.js anyhow, since the events themselves are effectively part of the Express.js API by extension of using this module).

from nodejs-depd.

Here is a quote from the Node.js project collaborators (the implementor of the deprecations emitting an event PR) on this point: nodejs/node#4782 (comment)

I split this out specifically because @dougwilson pointed out that depd already makes use of process.on('deprecation'). The idea is to ensure more alignment between core and what's happening in userland.

Ultimately Node.js core ended up overloading the warning event with all types of things being emitted there, not just deprecation events. The core basically parted ways with aligning to userland for unclear reasons.

from nodejs-depd.

Thanks for the clarification, seems pretty crappy that core went in a different direction than userland. That was a couple years ago though, and IMO the best way forward would be to try to standardize around the newer warning events - if that means it'll be a release or two with depd/express until it's fully complete, then so be it. Node.js < 6 is no longer maintained, so I don't see much reason to continue supporting unmaintained versions of node going forward in new major versions of depd or express.

from nodejs-depd.

I disagree.

from nodejs-depd.