chore: throw if more than one network config is passed #2056
Conversation
size-limit report 📦
|
|
Is this ready for review? Let's update the description |
|
@danisharora099 it's ready and I think description is up to date :D |
|
|
||
| const libp2pOptions = options?.libp2p ?? {}; | ||
| const peerDiscovery = libp2pOptions.peerDiscovery ?? []; | ||
|
|
||
| if (options?.defaultBootstrap) { | ||
| peerDiscovery.push(...defaultPeerDiscoveries(options.pubsubTopics)); | ||
| peerDiscovery.push(...defaultPeerDiscoveries(options.pubsubTopics!)); |
There was a problem hiding this comment.
Can we workaround not having to enforce its definition by adding a !?
There was a problem hiding this comment.
It is due to update happening inside of a function so TS don't pick it up and complains.
To address this I think we should fully rework the approach to options and my proposal is here with draft PR #2073
| if (options.contentTopics) { | ||
| options.shardInfo = { contentTopics: options.contentTopics }; | ||
| } | ||
|
|
||
| const shardInfo = options.shardInfo | ||
| ? ensureShardingConfigured(options.shardInfo) | ||
| : undefined; | ||
|
|
||
| options.pubsubTopics = shardInfo?.pubsubTopics ?? | ||
| options.pubsubTopics ?? [DefaultPubsubTopic]; |
There was a problem hiding this comment.
nit: Let's follow the same if-else syntax as you used for options.contentTopics for other two as well. Or the other way around with ? / : operators
There was a problem hiding this comment.
I think usage of ?? is the most convenient at this point as each of operators would require a condition
| if (flags.length > 1) { | ||
| throw Error( | ||
| "Too many network configurations provided. Pass only one of: pubsubTopic, contentTopics or shardInfo." | ||
| ); | ||
| } | ||
|
|
||
| logWhichShardInfoIsUsed(options); | ||
|
|
||
| if (options.contentTopics) { | ||
| options.shardInfo = { contentTopics: options.contentTopics }; | ||
| } | ||
|
|
||
| const shardInfo = options.shardInfo | ||
| ? ensureShardingConfigured(options.shardInfo) | ||
| : undefined; | ||
|
|
||
| options.pubsubTopics = shardInfo?.pubsubTopics ?? | ||
| options.pubsubTopics ?? [DefaultPubsubTopic]; | ||
|
|
||
| return shardInfo?.shardInfo; |
There was a problem hiding this comment.
Let's use the same if-else syntax for shardInfo and pubsubTopics as well, as used for contentTopics
There was a problem hiding this comment.
as I mentioned here - I don't think it is improvement for readability
if (shardInfo?.pubsubTopics) {
options.pubsubTopics = shardInfo.pubsubTopics;
} else if (!options.pubsubTopics) {
options.pubsubTopics = [DefaultPubsubTopic];
}
if (shardInfo?.shardInfo) {
return shardInfo.shardInfo;
}
return undefined;again, I think this area should be re-worked and here is the task #2073
| @@ -128,6 +117,37 @@ export async function createLibp2pAndUpdateOptions( | |||
| return libp2p; | |||
| } | |||
|
|
|||
| function configureNetworkOptions( | |||
| options: CreateWakuNodeOptions | |||
| ): ShardInfo | undefined { | |||
There was a problem hiding this comment.
Why does this return undefined optionally?
There was a problem hiding this comment.
because this is how types are defined and shardInfo can be missing in some cases
Problem
Users can provide more than one network config.
Solution
Throw
Notes