on now returns a deregistration function

[matthias-ccri]

Co-authored-by: Jason Miller developit@users.noreply.github.com

What is the purpose of this pull request? (put an "X" next to an item)

• Documentation update
• Bug fix
• Feature
• Code style update (formatting)
• Refactoring (no functional changes)
• CI related changes
• Other, please explain:

What changes did you make? (Give an overview)

The on function now returns a deregistration function that a consumer may call instead of off.

This pattern is present in other event emitter libraries, and it has a particular benefit that I care about. It allows consumers to avoid creating a variable to store the handler function. In other words, the handler function can be inlined into the on arguments. (in the current API, it is possible to inline the handler function, but then it is impossible to ever deregister it).

Allowing inlining via a deregistration function is a key API feature in my experience. I have seen bad things happen in enterprise codebases that were caused by developers creating handler function variables, which then degrade over time. Once the variable exists, future developers may decide to use it for other purposes. This is a classic maintenance risk where code loses its purpose and gets corrupted by multiple concerns. Inlining would prevent that. If the variable never exists in the first place, then the code would be more resilient to degradation over time.

That's the idea at least.

Is there anything you'd like reviewers to focus on?

The deregistration function is a good pattern, but it involves creating a new object (arrow function) on each invocation of on. I'm not well versed in JS engines but creating an object has implications for performance and GC.

In typical usage I don't think this is an issue, by hypothetically there may be high performance consumers who call on in high performance code loops, where allocating an object would degrade performance.

If the creation of the object is not acceptable for this library, then we may consider implementing two different on functions: one returning a deregistration function and one without (i.e. the current form). However, this would increase the library size a little.

If neither of these are valid for the design decisions of this library, then please feel free to close this PR.

Does this PR introduce a breaking change? (What changes might other developers need to make in their application due to this PR?)

Yes. The on function now returns a function instead of undefined. All consumers who relied upon the function returning undefined must update their code to ignore the return value.