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(network, DHT): stricter typing for event listeners in interfaces #2517

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

juslesan
Copy link
Contributor

@juslesan juslesan commented Apr 23, 2024

Summary

Stricter listener typing for events in the ITransport and Layer1Node interfaces.

It is no longer possible to set a listener with an inproper listener function for example:
layer1Node.on('manualRejoinRequired', (peerDescriptor: PeerDescriptor) => {})
will now be caught by the compiler.

Changes

  • DhtNode no longer export an Events type. It now only exports a DhtNodeEvents type that includes DhtNode and ITransport specific events.

@github-actions github-actions bot added network Related to Network Package dht Related to DHT package labels Apr 23, 2024
@github-actions github-actions bot added the utils label May 1, 2024
Copy link
Contributor

@teogeb teogeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do the on, off and once types need to be defined multiple times per function type.

This is no longer an issue, could remove it from the "Future improvements" list

packages/utils/src/EmitterOf.ts Outdated Show resolved Hide resolved
packages/utils/src/EmitterOf.ts Outdated Show resolved Hide resolved
type OverloadedListenerSetter<T> = Intersection<ListenerSetterTypes<T>>

// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
export type EmitterOf<T> = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This give bit strange error messages:

e.g.

const emitter: Layer1Node = {} as any
emitter.on('manualRejoinRequired', (foo: any) => {})
No overload matches this call.
  The last overload gave the following error.
    Argument of type '"manualRejoinRequired"' is not assignable to parameter of type '"ringContactRemoved"'.ts(2769)

Maybe good to add a bullet point about that into the "Future improvements" section

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems weird but I believe this is just overloading errors are reported in this case, as the assigned listener has any type in the first parameter.

@juslesan juslesan requested a review from teogeb May 7, 2024 08:39
Copy link
Collaborator

@ptesavol ptesavol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be merged to main.

@juslesan
Copy link
Contributor Author

juslesan commented Oct 1, 2024

Eslint broken here for some reason

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dht Related to DHT package network Related to Network Package utils
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants