alesgenova / post-me

📩 Use web Workers and other Windows through a simple Promise API
https://www.npmjs.com/package/post-me
MIT License
492 stars 13 forks source link

Add experimental debug feature to the Messenger #26

Closed alesgenova closed 3 years ago

alesgenova commented 3 years ago

I'm proposing an implementation to easily provide a visual debug of every message sent/received at the Messenger level. Let's start a discussion, this PR is not meant to be merged yet (will write docs and tests once we settle on an implementation).

Usage:

import {
  ParentHandshake,
  WindowMessenger,
  DebugMessenger,
} from 'ibridge';

import debug from 'debug';

let messenger = new WindowMessenger({
  localWindow: window,
  remoteWindow: childWindow,
  remoteOrigin: '*'
});

const log = debug('ibridge:parent');
messenger = DebugMessenger(messenger, log);

cost handshake = ParentHandshake({}, messenger);
...

What I like:

Here is a screenshot of the debug output of the demo application: ibridge_debug_output

franleplant commented 3 years ago

nice, I like this idea

franleplant commented 3 years ago

So thinking more about this I realize that I don't like it so much, the whole point of debug is flipping a switch and letting libraries output all the debugging information in whatever format they prefer but the proposed solution messes that up by allowing lib consumers to specify the debug prefix (i.e. ibridge:parent0) and I think that's not a good idea.

I would be fine if for example, the proposed decorator simply acted as a boolean flag and internally added any debug structuring we, lib maintainers, choose

messenger = DebugMessenger(messenger);

internally that will hook up debug with the proper prefixing and all that.

Although I like that the debug is on one place and can be tree shaken if not used I believe that this will have a severe limitations, for example, the more natural "id" for a given bridge side will be it's sessionId so we will need to address that somehow (since Messenger doesn't know anything about that). Additionally what if we want to debug other things beside messages? options? validations? any other operation?

I think that I would take the formatting you introduced here (with the emojis) and apply that to what old ibridge had, it's the simplest solution to the "debugging" problem (which ibridge already did solve).

And for the "avoid including the whole debug package from the bundle" we should be able via rollup (I know that on webpack that is possible) to replace the entire debug package with a noop for an optimized bundle variant (let's say the production bundle). So that also solves that additional problem.

I wouldn't spend too much time into this, unless we really want to revolutionize the way code debugs in Node.js / Javascript I would say let's keep to what most other libraries do, simply use debug whenever is needed inside the code and let's move on to more important things (such as browser level testing). It's really not that much of problem to sprinckle the code with debug statements since there's not always an easy way to abstract it, i.e. in this particular case we could abstract it only to the messenger layer but as stated before, what happens if we want to debug other stuff? Rust has similar features and it is widely accepted.

TLDR: use the new format with emojis I like but keep more or less the structure that old ibridge uses since it is the same thing that so many other libraries do and it's the obvious way of using debug.

alesgenova commented 3 years ago

So thinking more about this I realize that I don't like it so much, the whole point of debug is flipping a switch and letting libraries output all the debugging information in whatever format they prefer but the proposed solution messes that up by allowing lib consumers to specify the debug prefix (i.e. ibridge:parent0) and I think that's not a good idea.

Specifying the debug prefix can be useful though, for example in the demo app we open 5 connections, and makes it easy for the user to visually tell messages apart. I guess if you don't want to go for this implementation something similar can be done magically inside the library using the connection id (which is not known until the handshake).

Additionally what if we want to debug other things beside messages? options? validations? any other operation?

That's a good point, this type of debug as it is only allows for messages sent/received,

And for the "avoid including the whole debug package from the bundle" we should be able via rollup (I know that on webpack that is possible) to replace the entire debug package with a noop for an optimized bundle variant (let's say the production bundle). So that also solves that additional problem.

I liked the idea of the consumer injecting debug, so it's not a dependency and so a consumer could pass their own implementation (for example to capture debug messages during testing), but I don't feel strongly enough about it as to prevent us from adding debug. So yeah we can have additional outputs for the build process. Btw, even in a production build, we can still ship a lite version of debug. This function will behave just like the real default export of debug (minus colors and timing):


const debug = function(namespace: string, log?: (...data: any[]) => void) => {
log = log || console.debug || console.log || (() => {});
return (...data: any[]) => {
log!(namespace, ...data);
};
};

const log = debug('ibridge');