Fix ingress creation parameter inline documentation #296
Conversation
🦋 Changeset detectedLatest commit: 681011e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| */ | ||
| roomName?: string; | ||
| /** | ||
| * unique identity of the participant. optional | ||
| * unique identity of the participant. | ||
| */ | ||
| participantIdentity?: string; |
There was a problem hiding this comment.
since they are not optional, we shouldn't make them ?. and should move these to the top?
There was a problem hiding this comment.
Would that potentially break any existing code using the SDK?
There was a problem hiding this comment.
if this is required, then existing code would either have to set it, or it wouldn't be successful either way, right?
| @@ -67,11 +67,11 @@ export interface UpdateIngressOptions { | |||
| */ | |||
| name: string; | |||
| /** | |||
| * name of the room to send media to. optional | |||
| * name of the room to send media to. | |||
There was a problem hiding this comment.
this is optional right? on updates.
There was a problem hiding this comment.
Everything is optional in the dictionary in the sense that parameters left out will not be updated. It's odd to single out these 2 parameters. We could flag all parameters as optional, but this seems redundant on a parameter called "UpdateIngressOptions".
| @@ -67,11 +67,11 @@ export interface UpdateIngressOptions { | |||
| */ | |||
| name: string; | |||
| /** | |||
| * name of the room to send media to. optional | |||
| * name of the room to send media to. | |||
| name = opts.name || ''; | ||
| roomName = opts.roomName; | ||
| participantName = opts.participantName || ''; | ||
| participantIdentity = opts.participantIdentity || ''; |
There was a problem hiding this comment.
| participantIdentity = opts.participantIdentity || ''; | |
| participantIdentity = opts.participantIdentity; |
There was a problem hiding this comment.
I also want to fail if the parameter is specifically set, but to an empty string. Is there a better way to achieve this (beyond 2 tests in the if statement below)?
| throw new Error('options dictionary is required'); | ||
| } | ||
|
|
||
| name = opts.name || ''; |
There was a problem hiding this comment.
need consts in front of these
Co-authored-by: David Zhao <[email protected]>
No description provided.