chore: move protocols CreateOptions into interfaces and
#1145
Conversation
add possible TODO
size-limit report 📦
|
| @@ -66,18 +55,21 @@ export type UnsubscribeFunction = () => Promise<void>; | |||
| */ | |||
| class Filter implements IFilter { | |||
| multicodec: string; | |||
| pubSubTopic: string; | |||
| options: ProtocolCreateOptions; | |||
There was a problem hiding this comment.
Changelog update needed to highlight breaking change in API.
| /** | ||
| * Implements the [Waku v2 Light Push protocol](https://rfc.vac.dev/spec/19/). | ||
| */ | ||
| class LightPush implements ILightPush { | ||
| multicodec: string; | ||
| pubSubTopic: string; | ||
| options: ProtocolCreateOptions; |
There was a problem hiding this comment.
update changelog to highlight api breaking change
| super(components, options); | ||
| this.multicodecs = constants.RelayCodecs; | ||
|
|
||
| this.observers = new Map(); | ||
|
|
||
| this.pubSubTopic = options?.pubSubTopic ?? DefaultPubSubTopic; | ||
| this.options = options ?? {}; |
There was a problem hiding this comment.
you end up applyuing the default pubsub topic and every usage of this.option. This is cumbersome and prone to developer error.
Apply the default values in the constructor:
this.options = Object.assign({pubSubTopic: DefaultPubSubTopic}, options, {})
Or same logic in a getter get options(): ProtocolOptions
Or create a class that handles the ProtocolOptions and automatically apply default values
There was a problem hiding this comment.
TypeScript doesn't know if a particular value as been assigned to an object (``Object.assign) so would need to explicitly typecast it. Instead, I've replaced this.options` with a private `pubSubTopic` variable.
| @@ -88,7 +88,8 @@ export async function createLightNode( | |||
| const store = wakuStore(options); | |||
| const lightPush = wakuLightPush(options); | |||
| const filter = wakuFilter(options); | |||
| const peerExchange = wakuPeerExchange(options); | |||
|
|
|||
| // we can probably move `peerId` into `ProtocolCreateOptions` and remove `ProtocolOptions` and pass it in the constructor | ||
| // however, filter protocol can use multiple peers, so we need to think about this |
There was a problem hiding this comment.
All protocol can use multiple peers. I don't think PeerId belongs to CreateOptions.
Do note that it makes sense to have PubSubTopic in ProtocolOptions (too). Let's see how Waku scaling goes:
For now I'd remove this todo and leave it as it is and we can decide later once we ahve more information on sharding and Waku usage.
| @@ -88,7 +88,8 @@ export async function createLightNode( | |||
| const store = wakuStore(options); | |||
There was a problem hiding this comment.
You have not removed the CreateOptions definition in this file. I expect it to be replaced with CreateProtocolOPtions from @waku/interfaces
|
@fryorcraken the above have been addressed here #1153 |
* address comments from #1145 * fix: typedoc * address comments in #1146 (review) - update changelog - change naming for `EciesEncoderOptions` and `SymmetricEncoderOptions`
ref: #1000