-
Notifications
You must be signed in to change notification settings - Fork 156
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 typings for issue 690 #691
fix typings for issue 690 #691
Conversation
* in response to calling the OTPublisher.getRtcStatsReport() method. | ||
*/ | ||
rtcStatsReport?: CallbackWithParam<PublisherRtcStatsReport[]>, any>; | ||
rtcStatsReport?: CallbackWithParam<PublisherRtcStatsReport[], any>; |
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.
This is a required fix
@@ -420,7 +413,14 @@ declare module "opentok-react-native" { | |||
/** | |||
* https://github.com/opentok/opentok-react-native/blob/master/docs/OTPublisher.md | |||
*/ | |||
export class OTPublisher extends React.Component<OTPublisherProps> {} | |||
export class OTPublisher extends React.Component<OTPublisherProps, unknown> { |
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.
React.Component requires two type arguments: <Props, State>
I did not want to dive too deep in what should be declared for State; thus unknown
is a good start.
* in response to calling the OTSubscriber.getRtcStatsReport() method. | ||
*/ | ||
rtcStatsReport?: CallbackWithParam<RtcStatsReport>, any>; | ||
rtcStatsReport?: CallbackWithParam<RtcStatsReport, any>; |
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.
this is a required fix
@@ -551,9 +544,16 @@ declare module "opentok-react-native" { | |||
/** | |||
* https://github.com/opentok/opentok-react-native/blob/main/docs/OTSubscriber.md#custom-rendering-of-streams | |||
*/ | |||
export class OTSubscriberView extends React.Component<OTSubscriberViewProps> {} | |||
export class OTSubscriberView extends React.Component<OTSubscriberViewProps, unknown> {} |
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.
same:
React.Component requires two type arguments: <Props, State>
I did not want to dive too deep in what should be declared for State; thus unknown
is a good start.
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.
Thanks for the PR @MartinHarkins . I added a commit to rev the version and add a note to the change log. I am asking one other team member here to review this, and then we'll merge it in and ship it.
Contributing checklist
Solves issue(s)
Fixes: #690
The typings file was broken. Essentially the basic fixes needed are along those lines:
Another issue was with the recent typings added on
OTSessionProps
,OTSubscriberProps
andOTPublisherProps
Some methods were added onto the JS component, like
OTSession#getCapabilities
...However, they are not in fact
props
.The issue with those typings is that they force consumers of those three components, to fill in those props; despite the fact that they cannot be used anywhere in the component implementations.
The only reason they could be used was if a developer subclassed for example
OTSession
, and then called those props themselves.Those methods are a part of the components, but using them actually requires subclassing said components. There is no practical way to use them otherwise.
Additionally;
OTSession
,OTPublisher
andOTSubscriber
should probably have those functions removed from the declaredpropTypes
anddefaultProps
.I can do that but I need confirmation before I perform that change.
In anycase, if possible this PR could be merged and published quickly in order to fix the typings which are completely broken at the moment.