Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

feat: add content and peer routing progress events #355

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Mar 28, 2023

These are added as optional generics so this change should be non-breaking.

Refs: libp2p/js-libp2p#1652

These are added as optional generics so this change should be
non-breaking.
Comment on lines +91 to +98
export interface Libp2p<
FindPeerProgressEvents extends ProgressEvent = ProgressEvent,
GetClosestPeersProgressEvents extends ProgressEvent = ProgressEvent,
ProvideProgressEvents extends ProgressEvent = ProgressEvent,
FindProvidersProgressEvents extends ProgressEvent = ProgressEvent,
PutProgressEvents extends ProgressEvent = ProgressEvent,
GetProgressEvents extends ProgressEvent = ProgressEvent
> extends Startable, EventEmitter<Libp2pEvents> {
Copy link
Member

Choose a reason for hiding this comment

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

you should add generics to the extends ProgressEvent in order to appropriately populate progressEvent or else users won't be able to customize their data since it's always going to be set to ProgressEvent<any, unknown>

e.g.

Suggested change
export interface Libp2p<
FindPeerProgressEvents extends ProgressEvent = ProgressEvent,
GetClosestPeersProgressEvents extends ProgressEvent = ProgressEvent,
ProvideProgressEvents extends ProgressEvent = ProgressEvent,
FindProvidersProgressEvents extends ProgressEvent = ProgressEvent,
PutProgressEvents extends ProgressEvent = ProgressEvent,
GetProgressEvents extends ProgressEvent = ProgressEvent
> extends Startable, EventEmitter<Libp2pEvents> {
export interface Libp2p<
FindPeerT extends string = any, FindPeerD = unknown,
GetClosestPeersT extends string = any, GetClosestPeersD = unknown,
ProvideT extends string = any, ProvideD = unknown,
FindProvidersT extends string = any, FindProvidersD = unknown,
PutT extends string = any, PutD = unknown,
GetT extends string = any, GetD = unknown,
> extends Startable, EventEmitter<Libp2pEvents> {

this can obviously be improved, but without piping the generics through, you will lock types to any and unknown, and aren't getting much benefit

@@ -138,7 +146,7 @@ export interface Libp2p extends Startable, EventEmitter<Libp2pEvents> {
* }
* ```
*/
peerRouting: PeerRouting
peerRouting: PeerRouting<FindPeerProgressEvents, GetClosestPeersProgressEvents>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
peerRouting: PeerRouting<FindPeerProgressEvents, GetClosestPeersProgressEvents>
peerRouting: PeerRouting<ProgressEvent<FindPeerT, FindPeerD>, ProgressEvent<GetClosestPeersT, GetClosestPeersD>>

@@ -154,7 +162,7 @@ export interface Libp2p extends Startable, EventEmitter<Libp2pEvents> {
* }
* ```
*/
contentRouting: ContentRouting
contentRouting: ContentRouting<ProvideProgressEvents, FindProvidersProgressEvents, PutProgressEvents, GetProgressEvents>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
contentRouting: ContentRouting<ProvideProgressEvents, FindProvidersProgressEvents, PutProgressEvents, GetProgressEvents>
contentRouting: ContentRouting<ProgressEvent<ProvideT, ProvideD>, ProgressEvent<FindProvidersT, FindProvidersD>, ProgressEvent<PutT, PutD>, ProgressEvent<GetT, GetD>>

Comment on lines +6 to +9
export interface PeerRouting<
FindPeerProgressEvents extends ProgressEvent = ProgressEvent,
GetClosestPeersProgressEvents extends ProgressEvent = ProgressEvent,
> {
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants