Library upgrade and pull request review

[knighttower]

Hello @bcerati , @ozgurg , @theryaz, @Agillet ,

I stumbled with this library since I was looking to expand some libraries with a more comprehensive Bus system. I really liked the implementation and overall idea, not too big not too small. After using it, I found some bugs and minor things that IMO needed a little different approach to make it more suitable to more general use-cases while keeping most of the API as-is.
It also needed some maintenance, file cleaning, test unit and TS validation. So I decided to upgrade the library to what I thought was necessary and the version I created is substantially different than the current version, so it will need a revision and approval from you if you think is worth it to be incorporated. Here is the list:

Refactoring and Updates:

• Remove some unnecessary redundant methods to make it SOLID

• Remove different file language versions to unify the code into one and ease maintenance

• Fixed and made the code JsDocs and Ts compliant

• Fixed minor code issues

• Added correct distribution files to modules and browser

• Added Vitest unit test (passed)

• Added terser, webpack (for browser window export) and ts (for module export)

• Removed examples folder, the examples are now in the JsDocs comments for Docs creation

• Bump the version to Major since there was a lot of refactoring and changes in the Class API

• "emit" method now receives the 'context' as part of the 'args' only if set since it is not frequently used and allows to pass the data without the need to set "null" as the context, ex:

  • eventBus.emit('event.name', arg1, arg2, arg3)
  • eventBus.emit('event.name', arg1, arg2, {__context: YourInstance}, ...otherArgs) //order is not important

• Fixes to the wild card event match. It now accepts correct wild card in the "on" or "emit" method:

  • eventBus.emit('event.name.**') // matches on('event.name.hello') or on('event.name.hello.world')
  • eventBus.emit('event.*.string') // matches on('event.name.string') but not on('event.name.hello.world')
  • eventBus.on('event.*.string.**') // matches emit('event.name.string.emitter') or emit('event.name.string.hello.world')

Let me know if I can open the Pull Request and in the meantime here is the updated code.

Hope it wasn't too much, sometimes I get the OCD of coding :)
Thanks!

[soknifedev]

is this module still active? I would like to get this upgrade

[knighttower]

Hello @soknifedev , I'm still waiting for any of the maintainers to reply back. While we wait for them to give me the green light, the upgraded library git is available here: https://github.com/knighttower/js-event-bus, and I also uploaded to NPM : npm i @knighttower/js-event-bus which should also be available via CDN here: https://cdn.jsdelivr.net/npm/@knighttower/js-event-bus@latest/dist/browser/eventBus.min.js

[Shiva127]

Amazing job refactoring this library @knighttower. Well done!

My only issue is with __context being parsed in every argument.

Sometimes I transmit objects coming from external libraries and I do not want to have to sanitize everything that I pass to eventBus.emit().

You should really make two functions emit('event.name', ...args) and emitContext('event.name', this, ...args) and not parsing __context anywhere.

This is the only point making me not using your library…

[knighttower]

Hello @Shiva127.
You don't have to sanitize, the "__context", is automatically removed and injected into the "callback" if it is set. Here are some of the reasons why it was done like this:

1. Explicit Context Information: The explicit inclusion of the __context property makes it clear that the event is carrying additional context information, which can be helpful for developers using the API.
2. Consistency with Common Practice: Using a context object with the __context property aligns with common practices in JavaScript, such as the this keyword, which provides context for event handlers.
3. Flexibility for Future Enhancements: The context object allows for more flexibility in the future, allowing additional properties to be added without breaking existing code.
4. Clearer Documentation: Documentation for the API can clearly explain how to handle the context object and how to access the context information within event handlers.

The previous approach of using different arguments to indicate the presence of context information can lead to confusion and potential errors, especially for developers unfamiliar with the API. The explicit context object approach provides a more consistent and clear way to handle context information.

As for your suggestion, is an interesting suggestion. Having two separate functions, "emit" and "emitContext", would make it clearer when context information is being passed along with an event. However, there are a few potential drawbacks to consider:

• Increased API complexity: Introducing a second function would add to the complexity of the API, making it slightly more difficult for developers to learn and remember. This could increase the cognitive load on developers and make it more difficult to onboard new contributors.
• Potential redundancy: While it would make the distinction between context and non-context events more explicit, it could also introduce some redundancy, as the emitContext function would essentially be a wrapper around the emit function with additional context handling. This could lead to duplication of code and make the API less maintainable.
• Potential misinterpretation: If developers are not careful, they might misuse the emitContext function even when context is not necessary, leading to unnecessary complexity in their code. This could introduce performance overhead and make the application less efficient.
• Inconsistent with existing patterns: The proposed approach deviates from the existing pattern of using a single emit function with optional arguments to indicate the presence of context information. This could cause confusion for developers familiar with other libraries that follow this pattern.
• Documentation overhead: The introduction of a separate emitContext function would require additional documentation to explain its usage and purpose. This could increase the overall maintenance burden of the library.
• Reduced flexibility for future enhancements: Having two separate functions limits the flexibility for future enhancements. For instance, if additional context properties are needed in the future, it would require modifying both emit and emitContext functions.

Overall, while the proposed approach has the advantage of making the distinction between context and non-context events more explicit, it also introduces several drawbacks that could negatively impact the usability, maintainability, and consistency of the API.

I just updated the docs to add your comments and make the clear explanation of how the 'context' will be handled.

Thank you so much for your feedback. Please check out the updated docs here (https://github.com/knighttower/js-event-bus) for more feedback.

[Shiva127]

Thank you for your extensive response.

I maybe not have been precise enough, sorry. My concerns are the following:

1. Sometime I pass objects that have __context property in it (has you said, common practice) but should not be considered as the context of the event callback.

2. Sometime I pass objects, from my event callback, to an external function and therefore need the __context property to be maintained. This value will be deleted by your library.

3. What appends if you pass multiple arguments with each one having a __context? In your library, the last one will be used.

To resolve point 1, I have to rename the __context property to another name or delete it before calling event.emit().

To resolve point 2, I have to rename the __context property to another name or duplicate the property if I want to use it as effective context for the callback function before calling event.emit(), and later, rename again, in the callback function, the new named __context property to really have the __context property set for the future external function call.

I agree with you on the complexity, redundancy but I really think that not having an explicit argument to define the context of the callback function is a big mistake. Sorry.

Or maybe, the simplest solution is to keep only one function event.emit('event.name', null, ...args)…

[knighttower]

I'm curious on how your code is usually structured, perhaps if you share a code sample it may bring a better context to the discussion.

The "__context" property keyword was intentionally done with "double underscore" to avoid conflicts and collisions with standard objects, which follows common libraries' design standards. For instance let's look at Vue or jQuery sources:

Unless there is a very specific edge case that needs to use the prop "__context", an alternative would be to rename to "__contextInstance" or "__eventContext", let's take feedback on that.

"User" created objects, by standard, use only one underscore to define "private" or "protected" methods and properties. Libraries on the other hand, use "__" (double underscore), please see ->here and -->here for a brief explanation.
The concept of "__" was also borrowed from node.js to denote that this is a "reserved global variable" within the scope of the library and the event emitter. That's the inspiration to solve the issue of always settings "null" in every function instance and simplifying while allowing flexibility with the arguments, so that this would be achievable:

eventBus.emit('my-event');
eventBus.emit('my-event', ...args); // arguments only
eventBus.emit('my-event', 'a', 'b', 'c', {__context: new SomeObject()}); // Args and 'event context )

In my experience, most use cases will fall in the first and second example.

But let's see what others have to say, I'm all onboard to make this thing great!

[Shiva127]

There is no real context to my argumentation.

For me, binding a callback function to a parameter of a variadic argument object seems wrong.

I try to stay open and just try to find some "bad" edge case to your implementation confirming my instinct.

Maybe you’re right…

Sorry for wasting your time.

[knighttower]

@Shiva127 , you're not wasting my time, not at all. This is great to get different perspectives and use cases.
At the beginning when I saw the original implementation, I had the same question about the Rest Parameters (Variadic), but then I understood that this can be a good thing because it will allow to transfer the implementation to the developer rather than the library being too opinionated, I was Ok with it. I also wanted to stay away from having "null" parameters in the middle since is more error prone, harder to read and maintain. In cases where optional parameters are used, usually they are placed at the end of the function so that they are not obstructive or confusing, like this:

function hello(param1, param2, optional1 = null, optional2 = null);
// and implemented like this
hello(value1, value2) // the optional value(s) are truly optional

As for "transferring the implementation to the developer", it allows the dev to use it whichever the case requires:

function myFunction(callback) {
  callback({
    name: 'John Doe',
    age: 30,
    occupation: 'Software Engineer'
  });
}

myFunction(function(person) {
  console.log(person.name); // Prints 'John Doe'
  console.log(person.age); // Prints 30
  console.log(person.occupation); // Prints 'Software Engineer'
});

//Or
function myFunction(callback) {
  callback('John Doe', 30, 'Software Engineer');
}

myFunction(function(name, age, occupation) {
  console.log(name); // Prints 'John Doe'
  console.log(age); // Prints 30
  console.log(occupation); // Prints 'Software Engineer'
});

As for the "context", at first I did not see many use cases for it, but in edge cases the "this" instance may be beneficial if needed. It really is not needed to have it declared all the time, but if required it should be available. Since passing Rest parameters there is no real way to know which one is the "instance" (it could be easily confused with any other param that is an object) the best solution was to implement the declaration of "__context". That part makes it very "declarative" and readable while staying away from conflict.

Anyway, sorry for being so lengthy but I wanted to share most of the "why/how" the new upgrades became like this.

I think your comments and feedback are very valid and if you have any extra time, I would love to get more feedback on other projects I have created (https://github.com/knighttower).

Thanks! 😀