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

Introducing "dynamic mode" allowing enable/disable on the fly #156

Closed
wants to merge 7 commits into from

Conversation

dominicbarnes
Copy link

So, apologies in advance for the extra large PR. This is a feature I think that would be really useful, and I intend to solve several issues in one fell swoop. I'm also wide open to feedback on this, just let me know what you think!

Currently, the list of enabled/disabled namespaces is determined during init. Changing the list via .enable() and .disable() have no effect once a function has already been returned by debug(). (#150) This decision allows some optimization at a low-level, which is a good thing. (and why the feature I have here is opt-in)

For servers, it could be useful to turn on and off debugging for various features at will, without needing to restart the server to do so. This is what I had in mind while developing this feature, so you'll find a new example demonstrating the use-case. As an aside, I did not implement this feature for browsers yet, since it seems more like a server feature, but it would not be terribly complex to make them more consistent.

In dynamic mode, rather than returning a working enabled or disabled function, it returns a thin wrapper that checks .enabled(namespace) when invoked. This allows dynamically turning on and off various debug instances at will.

To enable dynamic mode, you can either use DEBUG_DYN=1 or debug.dynamic(true);, but the former is arguably easier. (since it will ensure all debug instances are created in dynamic mode)

To facilitate a dynamically-changing list of enabled/disabled namespaces, I needed to overhaul the underlying system. I've created able.js, (haha, terrible name, I'm open to alternatives!) along with a mocha test suite. (gives us a good start toward #104) It's a singleton lib that is a simple handler for things like .enable(), .disable(), .enabled() and a bit more. In addition, it includes a .stringify() method that generates a list like described in #19. (and a corresponding .parse() method for parsing user input)

All the fanfare around a distinct "dynamic mode" adds some complexity to the code, but this allows the already-optimized version to remain. If you guys are open to it, I'd like to investigate making this the default behavior, and probably benchmarking to see if there's a noticeable difference in performance.

I tried to be very careful with my changes here, although it was unfortunately difficult without any tests in place. I've tried out all the examples, and they appear to function as designed. Not only that, but I made a special effort to not change the current public API. I also tried to follow the existing code conventions, correct me where I've deviated.

tl;dr

  • "dynamic mode" allows changing enabled/disabled debugs on the fly (enabled via DEBUG_DYN=1 or debug.dynamic(true)
  • able.js is the file now responsible for managing the list of enabled/disabled namespaces (it also has tests)
  • currently this is opt-in, but I would like to hear thoughts on making it the default behavior
  • was careful not to change the public API, and examples were tested thoroughly to make sure it still works (but without proper tests, that was harder than it needed to be)

Dominic Barnes added 6 commits November 12, 2014 14:48
…own file

This adds the ability for namespaces to be enabled or disabled on the fly (aka: dynamic mode)
Rather than simply returning an enabled/disabled function, when dynamic mode is engaged, it
will instead return a thin wrapper that checks the enabled status before each and every call.

To accomodate this, the enable/disable lists needed to be upgraded. The new file (able.js)
is a singleton that facilitates this feature. Before, the list was really only capable of being
modified during init, but afterwards fell flat. This brings more capability there, and also
makes that specific feature more testable.
@lancejpollard
Copy link

+1 this sort of thing would make it easy to toggle on/off debugging for different thing in staging and production and stuff like that.

@lancejpollard
Copy link

@TooTallNate wondering what your thoughts are about this? could you see this being useful?

/**
* Checks if the specified `ns` is enabled.
*
* @param {String} ns Wildcards are not supported here

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for not supporting wildcards here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a really long time... but I think it was just for simplicity. Imagine I've enabled mocha* but disabled mocha:runner, what would you expect enabled("mocha*") to return? (since there is a clear exception to this rule that makes it so the wildcard technically can't be true 100% of the time)

@asheriff
Copy link

Yeah, after I posted my comment I dug into the code a bit. I was having that same dilemma as far as what enabled("mocha*") should return. Dunno if it would be simpler to have a single enabled array instead of enabled and disabled arrays.

@bydga
Copy link

bydga commented Feb 18, 2015

Im absolutely +1 for this PR. We are currently using it in our forked private npm module (wit hthis merged), but switching back to the "official" module would be nice :)

@pronebird
Copy link

Great idea. Having the same issue enabling debug on mocha, my dev builds are squashed blobs of javascript. I can't put enable() in between that easy.

@skogsmaskin
Copy link

+1

This will let us set up debugging in our deployed apps without bothering our ops guy.

@ibc
Copy link
Contributor

ibc commented Jul 3, 2015

Honestly I prefer my approach in #209. It allows creating "static" debug instances (as now) mixed with "dynamic" debug instances without breaking the current API.

@roccomuso
Copy link

+1

@thebigredgeek
Copy link
Contributor

To play devil's advocate, you could just set the environment variable at run time...

process.env.DEBUG=foo:*

@dominicbarnes
Copy link
Author

@thebigredgeek actually, no since whether or not the fn returned by debug() is a no-op or not is still determined on init. You'd have to modify process.env.DEBUG prior to your modules being loaded, and still be unable to affect them after the fact.

@thebigredgeek
Copy link
Contributor

@dominicbarnes can you update your code so that we can discuss further?

@thebigredgeek thebigredgeek added change-major This proposes or provides a change that requires a major release discussion This issue is requesting comments and discussion feature This proposes or provides a feature or enhancement labels Nov 14, 2016
@thebigredgeek
Copy link
Contributor

#209

@dominicbarnes
Copy link
Author

@thebigredgeek this PR turned 2 over the weekend, so it's been a very long time since I've looked at the code. I'll see if I can easily rebase this PR, but not sure when I'll get around to it.

@thebigredgeek thebigredgeek changed the base branch from master to v3 November 15, 2016 16:37
@thebigredgeek
Copy link
Contributor

Closing this for now

@thebigredgeek
Copy link
Contributor

#370 < V3 will add lots of these features.

@ibc
Copy link
Contributor

ibc commented Dec 14, 2016

Amazing. Thanks a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change-major This proposes or provides a change that requires a major release discussion This issue is requesting comments and discussion feature This proposes or provides a feature or enhancement
Development

Successfully merging this pull request may close these issues.

None yet

9 participants