on now returns a deregistration function - #215
matthias-ccri wants to merge 1 commit into
Conversation
馃 AI Pull Request Review (Antigravity Code Reviewer)Overall Risk: 馃煝 LOW RISK | Health Score: 馃搵 Executive SummaryThe PR introduces a convenient API enhancement to the mitt library by returning a deregistration function from the 'on' method. This pattern, commonly referred to as the 'off-callback' or 'cleanup' pattern, significantly improves developer ergonomics by allowing users to easily unsubscribe from events without needing to maintain a reference to the original handler function. The implementation is clean, idiomatic, and maintains backward compatibility with existing usage patterns. From an architectural perspective, the change is low-risk and aligns with modern JavaScript event-emitter patterns. By capturing the 'off' method via the 'mittInstance' reference, the implementation ensures that the returned function remains bound to the correct instance context. The code is well-structured, and the impact on the existing codebase is minimal, making it a safe and recommended addition to the library.
|
2 participants
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)
What changes did you make? (Give an overview)
The
onfunction now returns a deregistration function that a consumer may call instead ofoff.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
onarguments. (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
onin 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
onfunctions: 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
onfunction now returns a function instead ofundefined. All consumers who relied upon the function returningundefinedmust update their code to ignore the return value.