-
Notifications
You must be signed in to change notification settings - Fork 445
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: create has optional peer id type #875
Conversation
c8a937b
to
a754933
Compare
@@ -101,20 +103,22 @@ class Libp2p extends EventEmitter { | |||
*/ | |||
static async create (options) { | |||
if (options.peerId) { | |||
// @ts-ignore 'Libp2pOptions & CreateOptions' is not assignable to 'Libp2pOptions & constructorOptions' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe these Libp2pOptions
, CreateOptions
and constructorOptions
types need to be refactored to be more composable ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any suggestion? I think that the issue is that one says that the peerId is required while the other says it is optional. This causes:
Argument of type 'Libp2pOptions & CreateOptions' is not assignable to parameter of type 'Libp2pOptions & constructorOptions'.
Type 'Libp2pOptions & CreateOptions' is not assignable to type 'constructorOptions'.
Types of property 'peerId' are incompatible.
Type 'PeerId | undefined' is not assignable to type 'PeerId'.
Type 'undefined' is not assignable to type 'PeerId'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have been dealing with this type of stuff lately, specifically with constructor options that have options props but internally we have defaults for all or some.
So what im doing now is:
class Libp2p {
constructor(options: Libp2pOptions) {
const opts: Required<Libp2pOptions> = {...options, ...defaults}
}
}
This avoids having to truthy check everywhere, but also makes all options required so kinda forces you to have defaults for everything which might sounds troublesome but i think its good as a general rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thanks, for the ideas, we might be able to move into that in the future once we revamp libp2p config, but not now. This would mean we need to have libp2p modules as dependencies here.
When using the libp2p create static function, the
peerId
parameter is not required.I tried to simply make it optional in
CreateOptions
, but sadly it does not override the required inLibp2pOptions
. So, I needed to create a new typedef.Context #873