Skip to content
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

debug.enable() flushes enabled namespaces #425

Open
dbo opened this issue Feb 22, 2017 · 24 comments
Open

debug.enable() flushes enabled namespaces #425

dbo opened this issue Feb 22, 2017 · 24 comments
Labels
bug This issue identifies a malfunction help-wanted This issue has an actionable item pr-welcome This issue has an approved change; a pull request would be appreciated
Milestone

Comments

@dbo
Copy link

dbo commented Feb 22, 2017

Calling enable is disruptive to any enabled namespaces, which has changed in the recent minor version, caused by #409

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

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...

TooTallNate pushed a commit that referenced this issue Nov 8, 2017
Document enable, and help to avoid pitfall of #425
@Qix- Qix- added discussion This issue is requesting comments and discussion bug This issue identifies a malfunction labels Jun 20, 2018
@Qix-
Copy link
Member

Qix- commented Sep 11, 2018

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.

@Qix- Qix- added help-wanted This issue has an actionable item pr-welcome This issue has an approved change; a pull request would be appreciated and removed discussion This issue is requesting comments and discussion labels Sep 11, 2018
@geonanorch
Copy link

@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-
Copy link
Member

Qix- commented Sep 12, 2018

Well realistically both should be supported, I think.

@mblarsen
Copy link
Contributor

mblarsen commented Oct 6, 2018

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-
Copy link
Member

Qix- commented Oct 6, 2018

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

@mblarsen
Copy link
Contributor

mblarsen commented Oct 6, 2018

Related #451

@Qix-
Copy link
Member

Qix- commented Oct 6, 2018

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
Copy link
Contributor

mblarsen commented Oct 6, 2018

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

@Qix-
Copy link
Member

Qix- commented Oct 7, 2018

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
Copy link

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
Copy link

RobinBol commented Apr 8, 2020

Just pitching in, I have got a related problem:

My NPM module (called my-module) depends on debug@4.1.1, a dependency of my-module (called my-module-dep has the same debug@4.1.1 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 debug@4.1.1 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
Copy link

geonanorch commented Apr 8, 2020

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
Copy link

jehon commented Apr 9, 2020

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
Copy link

@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
Copy link

reggi commented Oct 4, 2020

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
Copy link

@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
Copy link

jehon commented Oct 6, 2020

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

@geonanorch
Copy link

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
Copy link

jehon commented Oct 10, 2020

@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-
Copy link
Member

Qix- commented Oct 10, 2020

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
Copy link

@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-
Copy link
Member

Qix- commented Oct 12, 2020

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

@mliguori-crestron
Copy link

mliguori-crestron commented Jun 15, 2021

Hi All, @geonanorch @Qix-

In version 4.3.1 of debug I have been trying to adopt the features of enable() and disable(). If I were to use the examples and add in additional printouts I can see it doesn't work.

let debug = require('debug');
debug.enable('foo:*,-foo:bar');
console.log('debug.enabled("foo")=', debug.enabled('foo'));
let namespaces = debug.disable();
debug.enable(namespaces);
debug('DO STUFF');

It seems like this bug listed here is at the source of this issue. Is this the problem you are talking about? And if yes, when would this build be released because it seems like it should come out soon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue identifies a malfunction help-wanted This issue has an actionable item pr-welcome This issue has an approved change; a pull request would be appreciated
Development

No branches or pull requests

9 participants