Debug: `debug.enable()` flushes enabled namespaces

[dbo]

Calling enable is disruptive to any enabled namespaces, which has changed in the recent minor version, caused by https://github.com/visionmedia/debug/pull/409

$ DEBUG=foo node -e 'var dbg = require("debug"); dbg.enable("bar"); console.log(dbg.enabled("foo"))'
=> false

[geonanorch]

The new implementation allows full control (i.e. override of the original DEBUG settings), which seems desireable (even more in the context of changing this dynamically later on, see issue #433 ).
I submit that the main problem here is that public method enable() is not documented.
Actually debug lacks an API reference document – does obviously not prevent it from being appreciated and widely used, but makes integration with other tools more difficult...

[Qix-]

I disagree with @geonanorch slightly - I feel like .enable() should be able to take -* and .disable() should be able to take * in order to clear the enabled namespaces.

In fact, the default behavior if no namespaces are passed (e.g. literally debug.enable() and debug.disable() alike) should be to implicitly pass *, which would enable/disable all namespaces, respectively.

[geonanorch]

@Qix- I don't think that we disagree, your proposal for a reset makes plenty of sense!
Personally I prefer the 'no parameter provided' approach to a wildcard, i.e. .enable() rather than .enable(*), but that is cosmetic really.

[Qix-]

Well realistically both should be supported, I think.

[mblarsen]

It seems this issue is a wont-fix? @Qix- @geonanorch ?

In other words: the bug is a feature :)

The implicit and explicit * and -* seems to be a different feature/issue.

[Qix-]

@mblarsen Definitely not a wontfix, it's just a breaking change and requires a lot of work.

[mblarsen]

Related #451

[Qix-]

Yeah, same thing really. The fact that .enable() overwrites _everything_ is what trips people up, methinks. Hence why I said debug.disable('*'); debug.enable(...) should be how users would migrate, and that calls to .enable() and .disable() should instead modify the existing namespaces.

In fact, I'd almost say that we get rid of .disable() as you merely need to negate any patterns passed to .enable() to get the exact same functionality in that case.

[mblarsen]

.disable() is nice for convenience and code readability though.

[Qix-]

But it's a redundant API call. Perhaps a better name for .enable(), then. Having multiple ways of doing the exact same thing is poor API design.

[360disrupt]

and that calls to .enable() and .disable() should instead modify the existing namespaces.

Actually, I got a use case for flushing debug. I have a mono repo with diff. modules in it. When I run unit tests it's useful to change env for each module in the tests. Then flush and enable debugging again with a diff setting:

require('dotenv').config({path: path.join(__dirname, root, '../src/mocha/env/.moduleA.test.env')});
require('debug').enable(process.env.DEBUG);

...

require('dotenv').config({path: path.join(__dirname, root, '../src/mocha/env/.moduleB.test.env')});
require('debug').enable(process.env.DEBUG);

[RobinBol]

Just pitching in, I have got a related problem:

My NPM module (called my-module) depends on [email protected], a dependency of my-module (called my-module-dep has the same [email protected] dependency. NPM will dedupe these two debug dependencies and only install one version of debug in the node_modules of my-module.

Now when my-module first enables debug logging and afterwards the dependency of my-module also enables it's own logging, the logging of my-module will be disabled because of the complete override of enable() on the same [email protected] node module:

// my-module
const debugMyModule = require('debug')('my-module');
require('debug').enable('my-module');
debugMyModule('test my module'); // enabled

// my-module-dep
const debugMyDepModule = require('debug')('my-dep-module');
require('debug').enable('my-dep-module');
debugMyDepModule('test my dep module'); // enabled

debugMyModule('test my module'); // disabled

Maybe the warning w.r.t. the complete DEBUG override for enable() could mention that this can cause unexpected interference with other modules.

Or even better, as proposed here (if I understand correctly), enable() should never disable any other enabled logging, merely extend the existing.

[geonanorch]

Mmmm... I feel in part responsible for the present situation, I'm afraid that I had not seen the full picture when I raised #433. Looking back at all the comments and related issues, my understanding is that:

• by default we want enable() to update the configuration, not replace it. There seem to be a large agreement on that point, so there is also a strong need for a fix
• we also want to have full control and be able to set "debug exactly 'this'", if only for convenience (only look at one component at a time) or for automation purpose (expect a specific output)
• finally there is a discussion on whether do keep disable() or use a negating pattern in enable(). Personally I agree with @mblarsen that keeping disable() is more readable, but (no offense @Qix- ) I have no qualms about providing 2 paths to the same outcome: internally they would be linked. Maybe a set() method is what we need to preempt that discussion ;-)

If we can agree on the above, then we could implement the following with minimal pain:

• enable(id) and disable(id) only update state for id, nothing else
• enable(=id) and disable(=id) set absolute state --with '*' as wildcard id to reset everything
• optionally set() can support all cases: set(id), set(-id), set(=id) (I am not pushing for set(), feels a bit like overkill, but cheap to add)

In all cases, clear documentation :-D

This issue has the label "pr-welcome", I am willing to do that if
a) there is an agreement (with or without set() ?)
b) nobody else is candidate (I have little time to offer)

[jehon]

I agree on the above.
Any function signature would be ok for me...

Note:

• disable(=id): I don't understand what it does?
• For my personal use, "set" and would not be necessary

Thinking futher (if you want to):

• Could also be: disableAll(), enable(id), disable(id)... ==> enable(=id) is for me disableAll() + enable(id). Would that not be more clear?
• This is a breaking change (and it should be nice to avoid those). So adding "disable(id)" and "enableAlso(id)" would not break anything... while enabling the expected behavior.

My two cents... if you want them.

[geonanorch]

@jehon thank you for providing feedback. I agree with you that disable(=id) is not clear, in fact it's horrible ;-)
With '=' I wanted to stay in line with Qix- initial comment and avoid adding new methods: adding the ---All() variants brings us to 4 methods, which is maybe not ideal either for clarity... But there are other ways. How about this revised proposal:

• enable(id) / disable(id) change only 'id'
• enable(*) / disable(*) enable / disable all
• enable() / disable() is obsolete but supported for backward-compatibility (if required?)

@Qix-, this is basically your old proposal, what do you think? If you want to make a call, someone can then start working on a PR (volunteers?...)

[reggi]

Having trouble with this today:

const a = DEBUG('alpha');
const b = DEBUG('beta');
DEBUG.enable('alpha');
DEBUG.enable('beta');
a('hi');
b('hi');

  beta hi +0ms

I'd really love a way to toggle on and off individual id's. We don't need to make a breaking change here I'd settle for enableId and disableId or any new method set.

Or even if I could access DEBUG.namespace and possibly do DEBUG.enable(`${DEBUG.namespace},alpha`) sort of hope $PATH works in bash?

[geonanorch]

@reggi yes the case is clear ;-)
In my previous comment I tried so summarize the discussion into a proposal, waiting for @Qix- to let us know how it should go.

[jehon]

@geonanorch
Water did pass under the bridge... I think we can go with the proposal :-)

[geonanorch]

Thanks @jehon . However I have no write access to that repo and would not risk preparing a PR without knowning if that is what @Qix- wants....

[jehon]

@geonanorch
Don't be afraid... I don't have access neither, but just clone the repo, work on your cloned repository, and then make a PR. That's easy. I have already done it on other open source project.
This may help: https://docs.github.com/en/free-pro-team@latest/github/collaborating-with-issues-and-pull-requests/creating-a-pull-request
When you make the pull request, you choose the visionmedia/debug - master as the target (aka this repo).

[Qix-]

I'm quite certain they meant they wanted to confirm the work that needs to be done, not that they don't know how to make a PR.

I've been neglecting this repo for a while now due to some pandemic-related stresses. I apologize.

I'm quite hesitant on changing anything at the present moment because of what the v5 release might look like. If we are to change the programmatic nature of things, I think we'll keep .enable and .disable as-is but deprecated in a minor release. Then, we'll introduce .configure() (better name suggestions are appreciated) to modify the enabled namespaces in a more intuitive way as I've described before.

I need to think a bit more about the best way to go about this given that v5 is going to be more or less a large rewrite. I also need to discuss some ownership matters with the other maintainers before I can say anything definitively.

Apologies for the vagueness. Things go slow with such a large and sensitive project.

[geonanorch]

@jehon as Qix surmised I raised PRs before ;-) Thanks for the intention, though (you might want to PM in such cases, which are not directly relevant to an issue)
@Qix- no problem at all. Github is full of repos where the owner no longer answers, so I for one am glad you got back to us. And understood, let's wait and see what v5 brings!
Should this issue be closed or requalified, then?

[Qix-]

I'll close this when it's resolved in the new version. :)