This repository was archived by the owner on Jun 26, 2023. It is now read-only.
feat: add content and peer routing progress events#355
Draft
achingbrain wants to merge 1 commit intomasterfrom
Draft
feat: add content and peer routing progress events#355achingbrain wants to merge 1 commit intomasterfrom
achingbrain wants to merge 1 commit intomasterfrom
Conversation
These are added as optional generics so this change should be non-breaking.
SgtPooki
suggested changes
Mar 31, 2023
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> { |
Member
There was a problem hiding this comment.
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
| * ``` | ||
| */ | ||
| peerRouting: PeerRouting | ||
| peerRouting: PeerRouting<FindPeerProgressEvents, GetClosestPeersProgressEvents> |
Member
There was a problem hiding this comment.
Suggested change
| peerRouting: PeerRouting<FindPeerProgressEvents, GetClosestPeersProgressEvents> | |
| peerRouting: PeerRouting<ProgressEvent<FindPeerT, FindPeerD>, ProgressEvent<GetClosestPeersT, GetClosestPeersD>> |
| * ``` | ||
| */ | ||
| contentRouting: ContentRouting | ||
| contentRouting: ContentRouting<ProvideProgressEvents, FindProvidersProgressEvents, PutProgressEvents, GetProgressEvents> |
Member
There was a problem hiding this comment.
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, | ||
| > { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These are added as optional generics so this change should be non-breaking.
Refs: libp2p/js-libp2p#1652