Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: rename event emitter class #2173

Merged
merged 3 commits into from
Oct 25, 2023
Merged

Conversation

achingbrain
Copy link
Member

We add types to a EventEmitter class which extends the plain js EventTarget.

The name is chosen to give familiarity to node developers but it's not generally a good idea.

This refactor:

  1. Exports a new TypedEventTarget interface which adds types to EventTarget
  2. Renames EventEmitter to TypedEventEmitter to draw a distinction between them
  3. Re-exports TypedEventEmitter as EventEmitter to preserve backwards compatibility

In the future all consuming code should rely on the TypedEventTarget interface while implementing code uses TypedEventEmitter as an implementation of a TypedEventEmitter.

By depending on the interface and not the implementation consuming code is isolated from problems that arise when two versions of @libp2p/interface is in the dependency tree and the event subsystems would otherwise be compatible due to type overlap.

This manifests as an error message about incompatible implementations of the private #listeners field in EventEmitter.

Backwards compatibility is achieved by exporting the TypedEventEmitter class as the older EventEmitter name. This should be removed in a future PR to the v1.0 branch once external modules (e.g. Gossipsub) have been updated to use the new TypedEventEmitter symbol.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

We add types to a EventEmitter class which extends the plain js EventTarget.

The name is chosen to give familiarity to node developers but it's
not generally a good idea.

This refactor:

1. Exports a new TypedEventTarget interface which adds types to EventTarget
2. Renames EventEmitter to TypedEventEmitter to draw a distinction between them

In the future all consuming code should rely on the TypedEventTarget interface
while implemmenting code uses TypedEventEmitter as an implementation of
a TypedEventEmitter.

By depending on the interface and not the implementation consuming code is
isolated from problems that arise when two versions of `@libp2p/interface`
is in the dependency tree and the event subsystems would otherwise be
compatible due to type overlap.

This maifests as an error message about incompatible implementations of
the private `#listeners` field in EventEmitter.
@achingbrain achingbrain requested a review from a team as a code owner October 25, 2023 14:38
@achingbrain achingbrain merged commit 50f912c into master Oct 25, 2023
2 checks passed
@achingbrain achingbrain deleted the refactor/rename-event-emitter branch October 25, 2023 14:39
@MysticalDragon98
Copy link

Hi, this is firing an error in my implementation:

import { TypedEventEmitter, CustomEvent } from '@libp2p/interface/events';
         ^^^^^^^^^^^^^^^^^
SyntaxError: The requested module '@libp2p/interface/events' does not provide an export named 'TypedEventEmitter'

And checking in the 0.1.3 version, there is no TypedEventEmitter published in the npm registry version. Maybe you updated the libp2p but not the interface in the npm registry?

@achingbrain
Copy link
Member Author

The original version of this PR went out with the refactor: tag which release-please didn't translate into modules needing a release so @libp2p/interface had the TypedEventEmitter class added but it was not released.

Hold tight, a new version should be out shortly that fixes the issue.

@achingbrain
Copy link
Member Author

@MysticalDragon98 this should be fixed now, sorry for the disruption. Please delete any lockfiles & reinstall deps.

This was referenced Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants