Add debug functionality #53

sbencoding · 2020-03-25T20:34:10Z

This PR aims to implement a built-in debug feature described in #2

How it's implemented

There's a static function in the Emittery class called debugLogger, that takes in 4 parameters

type - type of debug log
debugName - set by the instance of Emittery that generated this log
eventName - event name related to the log
eventData - in case of emit, this is the data sent by it

Debugging can be triggered in 3 ways currently:

the DEBUG environment variable is set to emittery or *
the static isDebug function in Emittery returns true (by default this handles the environment variable, but this function can be overridden, to enable package wide debugging instead of the 3rd method.
the isDebug variable is set to true on one or more instances.

Questions/suggestions

I wasn't sure if the default logger function should include some sort of timestamp.

It might add too much complexity but a verbosity option could be nice. For example currently every emit, subscribe, unsubscribe etc. event is logged, but in some cases they might not all be interesting. Of course this can be achieved by overriding the debugLogger function on Emittery, but it would be a nice convenience option.

Is it acceptable that there's one static logging function on Emittery or should the individual instanced of it be able to override this function. This would add the ability for different instances of Emittery to log differently based on their function in an application.

Fixes #2

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

#2: Built-in debug info

IssueHunt has been backed by the following sponsors. Become a sponsor

sbencoding · 2020-03-25T21:04:25Z

Not sure why codecov doesn't pickup that I test the debugLogger static function on Emittery.
Unfortunately I'm not familiar with this software, but I tried calling the method directly from one of the test cases and it still showed that I missed that function :(

sindresorhus · 2020-08-10T19:55:45Z

index.js

@@ -181,16 +181,37 @@ class Emittery {
 		};
 	}

-	constructor() {
+	static isDebug() {


This should be a getter.

sindresorhus · 2020-08-10T19:56:19Z

index.js

+
+	static debugLogger(type, debugName, eventName, eventData) {
+		if (typeof eventName !== 'string') {
+			eventName = eventName.toString();


This is moot. Template interpolation already calls toString.

sindresorhus · 2020-08-10T19:59:24Z

Questions/suggestions

Share your opinion on these questions (with reasonings).

sbencoding · 2020-08-11T15:24:32Z

I've implemented the requested changes, updated documentation and tests where it was needed.
My thoughts on the questions:

Timestamps

I think it would be useful only in specific scenarios if the default logging function printed out timestamps as well. The use case I can currently think of where time is relevant is an application where an emit happens periodically e.g. every 10 seconds. I think a minutes:seconds:milliseconds format would be the best in this case, since I don't expect the debug to run for hours or days.
Because I can't think of better/more use cases I don't think this would be useful to have it in by default.

Verbosity filter

I think this is useful because with multiple events and multiple instances of Emittery the logs could get cluttered very quickly if we log every subscribe/unsubscribe event too. I can think of two approaches that would be good here.

Verbosity levels from 1 to 3
Where 1 only logs emits, 2 logs emits and sub/unsub, and 3 logs everything
An array of strings that defines the event types we're interested in
For example we could add emit, subscribe and clear to it, if those are the events we're interested in. This allows for more precise control than option number 1 but also takes a bit more time to set up.

The other thing that needs to be decided here is where we implement the verbosity setting, in the Emittery class, so it affects all instances at once, or on a per instance level, so it's more precisely controllable. However on the per instance level we would have to manually update every instance if we wanted to change the setting for all instances.
Overall I would say that implementing it globally is better because it would be more efficient at reducing the overall clutter.

Static logger function

I think it would be better to have this on a per instance basis, but I guess this question is kind of the same as the implementation of the verbosity filter. The benefit of logging on a per instance basis would be that the different instances could access potentially useful data that is only available in their context. But again the drawback is that we lose the ability to update the logging function of all instances all at once.
Overall I think implementing this on a per instance basis would provide more functionality than it would take away. I couldn't come up with a use case where updating every logger function would be that important, but I can imagine that some instances would like to log information only available in their context.

sindresorhus · 2021-03-07T10:38:08Z

Sorry, I just realized I missed the notification for this.

sindresorhus · 2021-03-07T10:38:12Z

Timestamps

I think timestamp can be useful for comparing how far from eachother certain events are. I agree, the date is not very useful, but it should follow the ISO9601 minus the date, so hours:minutes:seconds.milliseconds. I think timestamps should be on by default. We can add an option to remove based on future requests.

Verbosity filter

I'm not sold on this. I think it's better to just be verbose by default and we can add modes to make it less verbose when users requests it from real-world experience.

Static logger function

Yeah, it should be on the instance.

I think it should be {debug: {name: 'foo'}}, not {debugName: 'foo'}. The former lets us add more debug-related options later on.

The docs and TS docs comments needs more work regarding formatting and typos.

sbencoding · 2021-03-20T16:39:30Z

Here are some of the changes I've made:

name for debugging is passed as part of an object that contains debugging setup data in the constructor
debugLogger is no longer static, instead logger is set in the debug options
isDebug that was on the instance is now set in debug options as well. The global isDebug that controls debugging for all instances is kept as it was before
Add timestamp to the default logger function
Update tests
Update documentation (both readme and typescript)

Let me know if I've missed anything, or there's something that could be done in a better way

P.S. finally I pass all the checks yay!

sindresorhus · 2021-05-19T07:48:54Z

index.d.ts

+
+To enable this feature set the DEBUG environment variable to 'emittery' or '*'. Additionally you can set the static `isDebug` variable to true on the Emittery class, or `myEmitter.debug.enabled` on an instance of it for debugging a single instance.
+
+See API for more information on how debugging works


All sentences should end with a .

sindresorhus · 2021-05-19T07:49:42Z

index.d.ts

+
+See API for more information on how debugging works
+*/
+type DebugLogger = (type: string, debugName: string, eventName?: EventName, eventData?: Record<string, any>) => void;


The event data here should be strongly-typed.

sindresorhus · 2021-05-19T07:51:12Z

index.d.ts

+	const emitter = new Emittery({debug: {name: 'myEmitter'}});
+	emitter.on('test', data => { // do something });
+	//=> [16:43:20.417][emittery:subscribe][myEmitter] Event Name: test
+	//	data: undefined


This is too cramped. Use more empty lines to make it more readable. The example should also include the part that emits the event. Generally, code examples should be runnable on their own.

Applies to all code examples.

sindresorhus · 2021-05-19T07:51:33Z

index.d.ts

+	/**
+	Define a name for the instance of Emittery to use when outputting debug data.
+
+	@default Empty string


This is not a valid value for @default.

sindresorhus · 2021-05-19T07:51:42Z

index.d.ts

+	//	data: undefined
+	```
+	*/
+	name: string;


Suggested change

name: string;

readonly name: string;

sindresorhus · 2021-05-19T07:55:23Z

index.js

@@ -9,6 +9,8 @@ const resolvedPromise = Promise.resolve();
 const listenerAdded = Symbol('listenerAdded');
 const listenerRemoved = Symbol('listenerRemoved');

+let globalDebugFlag = false;


Suggested change

let globalDebugFlag = false;

let isGlobalDebugEnabled = false;

sindresorhus · 2021-05-19T07:55:49Z

index.js

@@ -189,10 +191,42 @@ class Emittery {
 		};
 	}

-	constructor() {
+	static get isDebug() {


Suggested change

static get isDebug() {

static get isDebugEnabled() {

Just to make it clear.

sindresorhus · 2021-05-19T07:56:05Z

index.js

+
+		if (!this.debug.enabled) {
+			this.debug.enabled = false;
+		}


I don't get what the point of this is.

sindresorhus · 2021-05-19T07:57:05Z

index.js

@@ -219,6 +257,10 @@ class Emittery {
 			assertEventName(eventName);
 			getListeners(this, eventName).delete(listener);

+			if (Emittery.isDebug || this.debug.enabled) {
+				this.debug.logger('unsubscribe', this.debug.name, eventName, undefined);
+			}


This is repeated many times. There should be a utility method for this.

sindresorhus · 2021-05-19T07:57:46Z

readme.md

+### isDebug
+Enables/Disables debug mode for all instances


Suggested change

### isDebug

Enables/Disables debug mode for all instances

### isDebug

Toggle debug mode for all instances.

Formatting

sindresorhus · 2021-05-19T07:58:44Z

The code looks good, but the docs formatting and text needs more work.

sbencoding · 2021-05-23T11:09:49Z

I implemented the requested changes

I wasn't sure how to make DebugLogger in index.d.ts strongly-typed properly, but I've tried, please let me know if there's a better way to do this, or if how I did it is correct at all.
I couldn't remove the following code

if (typeof eventName === 'symbol') {
        eventName = eventName.toString();
}

Because it throws TypeError: Cannot convert a Symbol value to a string when the string interpolation happens.
I've tried a small, isolated example:

let s = Symbol('test');
console.log(`Symbol to string: ${s}`);

This results in the same error, so I don't know if there's a way to work around this.

sindresorhus · 2021-05-25T13:11:30Z

readme.md


 ###### name

 Type: `string`

-Default: `Empty string`
+Default: `null`


I prefer using undefined over null.

sindresorhus · 2021-05-25T13:13:26Z

readme.md

 emitter.on('test', data => { // do something });
+emitter.emit('test');


Readme and index.d.ts should be in sync.

sindresorhus · 2021-05-25T13:14:59Z

I wasn't sure how to make DebugLogger in index.d.ts strongly-typed properly, but I've tried, please let me know if there's a better way to do this, or if how I did it is correct at all.

I would recommend writing some type tests to confirm it's correct: https://github.com/sindresorhus/emittery/blob/main/index.test-d.ts

I couldn't remove the following code

Ah yeah. I forgot it's not interpolatable.

sbencoding · 2021-05-27T10:01:33Z

I've implemented the changes, and wrote some type tests for the strongly-typed logger

sindresorhus · 2021-05-28T14:14:16Z

Looks good now. Thanks :)

sbencoding added 6 commits March 25, 2020 18:13

Implement debug logging

5697ebb

Add tests for isDebug option

4bed384

Update type definition and add type testing for isDebug option

137f3d0

Sync readme with type definition

7824ecc

Fix readme formatting

984092d

Even more readme new line fixes

2e0d8e9

sindresorhus requested changes Aug 10, 2020

View reviewed changes

sbencoding added 6 commits August 11, 2020 15:03

Merge with upstream master

8c54c73

Remove the subscribeOnce log message

3786382

Make the global isDebug flag a getter

342527e

global isDebug flag can be set during runtime

76dce93

Update documentation with new usage of the global isDebug flag

89188ef

Remove unnecessary string conversion

833dea3

Base automatically changed from master to main January 23, 2021 10:58

sbencoding added 7 commits March 20, 2021 14:11

Merge with upstream/main

e1ef55c

Move debugLogger to instance, add time logging by default

8dc216d

Move isDebug of instance to constructor options

4601375

Add test for setting instance debugging without constructor

38fc1b6

Move logger function to constructor debug options

7aa5595

Update documentation with changes to debug API

1195122

Fix readme mistakes (format + typo)

c54319f

sindresorhus requested changes May 19, 2021

View reviewed changes

Improve formatting of docs and make DebugLogger strongly typed

d0daaa5

sbencoding added 3 commits May 22, 2021 22:04

Minor refactoring

78daf99

Update docs for refactored variable name

19cbdd1

Fix introduced test problems

1e8569d

sindresorhus reviewed May 25, 2021

View reviewed changes

readme.md Outdated

###### name

Type: `string`

Default: `Empty string`

Default: `null`

Copy link

Owner

sindresorhus May 25, 2021

Choose a reason for hiding this comment

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

I prefer using undefined over null.

sindresorhus reviewed May 25, 2021

View reviewed changes

readme.md

emitter.on('test', data => { // do something });

emitter.emit('test');

Copy link

Owner

sindresorhus May 25, 2021

Choose a reason for hiding this comment

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

Readme and index.d.ts should be in sync.

sbencoding added 3 commits May 27, 2021 10:24

Make debug name undefined by default, fix default for debug enabled doc

726ca18

Sync documentations

13398fc

Add more type definition tests for debug feature

523ef5e

Update readme.md

d4e9cce

sindresorhus changed the title ~~Implements debug feature as described in #2~~ Add debug functionality May 28, 2021

sindresorhus merged commit 43b6deb into sindresorhus:main May 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add debug functionality #53

Add debug functionality #53

sbencoding commented Mar 25, 2020 •

edited by sindresorhus

Loading

sbencoding commented Mar 25, 2020

sindresorhus Aug 10, 2020

sindresorhus Aug 10, 2020

sindresorhus commented Aug 10, 2020 •

edited

Loading

sbencoding commented Aug 11, 2020

sindresorhus commented Mar 7, 2021

sindresorhus commented Mar 7, 2021

sbencoding commented Mar 20, 2021

sindresorhus May 19, 2021

sindresorhus May 19, 2021

sindresorhus May 19, 2021

sindresorhus May 19, 2021

sindresorhus May 19, 2021

sindresorhus May 19, 2021

sindresorhus May 19, 2021

sindresorhus May 19, 2021

sindresorhus May 19, 2021

sindresorhus May 19, 2021

sindresorhus commented May 19, 2021

sbencoding commented May 23, 2021

sindresorhus May 25, 2021

sindresorhus May 25, 2021

sindresorhus commented May 25, 2021

sbencoding commented May 27, 2021

sindresorhus commented May 28, 2021


		To enable this feature set the DEBUG environment variable to 'emittery' or '*'. Additionally you can set the static `isDebug` variable to true on the Emittery class, or `myEmitter.debug.enabled` on an instance of it for debugging a single instance.

		See API for more information on how debugging works

	let globalDebugFlag = false;
	let isGlobalDebugEnabled = false;

		emitter.on('test', data => { // do something });
		emitter.emit('test');

Add debug functionality #53

Add debug functionality #53

Conversation

sbencoding commented Mar 25, 2020 • edited by sindresorhus Loading

How it's implemented

Questions/suggestions

Referenced issues

sbencoding commented Mar 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Aug 10, 2020 • edited Loading

sbencoding commented Aug 11, 2020

Timestamps

Verbosity filter

Static logger function

sindresorhus commented Mar 7, 2021

sindresorhus commented Mar 7, 2021

sbencoding commented Mar 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented May 19, 2021

sbencoding commented May 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented May 25, 2021

sbencoding commented May 27, 2021

sindresorhus commented May 28, 2021

sbencoding commented Mar 25, 2020 •

edited by sindresorhus

Loading

sindresorhus commented Aug 10, 2020 •

edited

Loading