Skip to content
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

New OpenTok React Native API #289

Closed
msach22 opened this issue May 28, 2019 · 14 comments
Closed

New OpenTok React Native API #289

msach22 opened this issue May 28, 2019 · 14 comments

Comments

@msach22
Copy link
Contributor

msach22 commented May 28, 2019

This is a proposal for a new API that would allow more control over rendering OTSubscriber components. Currently, the library assumes that you want to subscribe to all streams, but this new API would allow you to subscribe and render views individually. The API proposal also includes a sample on how to style, listen to events, and control subscriber audio and video.

import React, { Component } from 'react';
import { View } from 'react-native';
import { OTSession, OTPublisher, OTSubscriber } from 'opentok-react-native';

export default class App extends Component {
  constructor(props) {
    super(props);
    this.apiKey = '';
    this.sessionId = '';
    this.token = '';
    this.state = {
        subscribers: [],
        subscriberEventHandlers: {},
        subscriberProperties: {},
    }
    this.sessionEventHandlers = {
     streamCreated: event => {
         this.setState((state) => {
             const subscribersCopy = [... state.subscribers];
             subscribersCopy.push(event.streamId);
             const subscriberEventHandlers = state.subscriberEventHandlers;
             subscriberEventHandlers[event.streamId] = {
                connected: event => {
                    console.log('connected event');
                }
             }

             const subscriberProperties = state.subscriberProperties;
             subscriberProperties[event.streamId] = {
                 subscribeToAudio: false,
                 subscribeToVideo: true,
             }
             return {
                 subscribers: subscribersCopy,
                 subscriberEventHandlers,
                 subscriberProperties,
             }
         })
     },
     streamDestroyed: event => {
         const { subscribers, subscriberEventHandlers } = this.state;
         const subscribersCopy = {... subscribers};
         const streamIndex = subscribersCopy.indexOf(event.streamId);
         if (streamIndex > -1) {
             subscribersCopy.splice(streamIndex, 1);
             this.setState((state) => {
                 const { subscriberEventHandlers, subscriberProperties } = state;
                 if (subscriberEventHandlers && subscriberEventHandlers[event.streamId]) {
                     delete subscriberEventHandlers[event.streamId];
                 }
                 if (subscriberProperties && subscriberProperties[event.streamId]) {
                     delete subscriberProperties[event.streamId];
                 }
                 return {
                     subscribers: subscribersCopy,
                     subscriberEventHandlers,
                     subscriberProperties,
                 }
             })
         }
     }
   } 
  }

  render() {
    const { subscribers, subscriberEventHandlers, subscriberProperties } = this.state;
    const otSubscribers = subscribers.map((streamId, index) => {
        return <OTSubscriber key={index} streamId={streamId} style={{ width: 100, height: 100 }} eventHandlers={subscriberEventHandlers[streamId]} properties={{
      subscribeToVideo: subscriberProperties[streamId].subscribeToVideo,
      subscribeToAudio: subscriberProperties[streamId].subscribeToAudio,
   }} />
    });
    return (
      <View style={{ flex: 1, flexDirection: 'row' }}>
        <OTSession apiKey={this.apiKey} sessionId={this.sessionId} token={this.token}>
          <OTPublisher style={{ width: 100, height: 100 }} />
          { otSubscribers }
        </OTSession>
      </View>
    );
@msach22
Copy link
Contributor Author

msach22 commented May 28, 2019

@ggoldens @evillemez @ArtemSerga Would love your thoughts on this.

@ggoldens
Copy link
Contributor

LGTM @msach22 !

@ArtemSerga
Copy link
Contributor

In generate API is the same as was, but with some low-level control around steamid

  1. in streamCreated callback we can started manually control subscription per every steamid(subscriberEventHandlers[event.streamId], subscriberProperties[event.streamId]).
    For me It looks good, understandable and logical.
    P.S. subscribers: state.subscribers.push(event.streamId) will not work because push method won't return a new Array.

  2. streamDestroyed looks complicated. I understood that this is just an opposite operation for streamCreated, but when you have 2-3-5 places with different Opentok sessions - things become verbose and uncomfortable for reading.

So, I have only one feedback - add in library some "helper" functions to hide implementation details in streamCreated and streamDestroyed.

@msach22 , I'm so glad to see a first steps to multiple sessions support! Thanks!

@msach22
Copy link
Contributor Author

msach22 commented May 29, 2019

@ArtemSerga Thanks for the feedback.
Yes, more granularity is the goal here.

  1. Yes, granularity is the goal here. Thanks for pointing out the issue with the array, I fixed it the sample above 😄.
  2. We can have a default behavior where when a stream is destroyed, the views are automatically destroyed and the events will also stop, but the application owner would be responsible for freeing up the state. Keep in mind that I only showed all these options as a sample, but you only really need a subscribers array that you can iterate over to render the OTSubscriber components. The eventHandlers, properties, etc are optional, but allow more control.

What did you have in mind for the helper functions?

@ArtemSerga
Copy link
Contributor

Helper function to hide verbose code

in streamCreated callback, i.e:

import { addStream } from "opentok-react-native"; 
const { subscribers, subscriberEventHandlers, subscriberProperties } = addStream(event);

and in streamDestroyed callback, i.e:

import { removeStream } from "opentok-react-native"; 
const { subscribers, subscriberEventHandlers, subscriberProperties } = removeStream(event)

And then, a developer can decide what to do with already calculated subscribers, subscriberEventHandlers, subscriberProperties . Process with setState, redux or other state management approach.

@knavu2
Copy link

knavu2 commented May 31, 2019

I'm super excited to hear that you want to expand the API to provide more control and flexibility over how streams are rendered! With that said, I believe that we're mixing concerns here, specifically, session state handling and stream rendering. A consequence of this is also that the API becomes unnecessarily verbose. Instead I would prefer to see this implemented as a render prop component (e.g., <OTSusbcribers>) that handles state for us so that we only have to concern ourselves with how the streams are rendered, as per the example below.

import React, { Component } from 'react';
import { View } from 'react-native';
import { OTSession, OTPublisher, OTSubscriber } from 'opentok-react-native';

export default class App extends Component {
    state = {
        streamIdsWeWantToHide: ['some-stream-id']
    }

    renderSubscriber(subscriber) {
        const {
            subscriberId, 
            subscriberProperties
        } = subscriber;

        const subscriberEventHandlers = {
            onClick: (event) => this.setState(state => ({
                streamIdsWeWantToHide: [...state.streamIdsWeWantToHide, event.streamId]
            })),
            connected: (event) => console.log('connected event')
        }
    
        const hideStream = this.state.streamIdsWeWantToHide.includes(subscriberId);
        
        return (
            <OTSubscriber 
                streamId={subscriberId}
                properties={{
                    ...subscriberProperties,
                    style: {
                        height: 100,
                        width: 100,
                        opacity: hideStream ? 0 : 1
                    }
                }}
                eventHandlers={subscriberEventHandlers}
            />
        )
    }

    render() {
        return (
            <View style={styles.containerStyle}>
                <OTSession apiKey="API_KEY" sessionId="SESSION_ID" token="TOKEN">
                    <OTPublisher style={styles.publisherStyle} />
                    <OTSubscribers>
                        {subscribers => subscribers.map(this.renderSubscriber)}
                    </OTSubscribers>
                </OTSession>
            </View>
        )
    }
}

or something similar.

Again, so happy to see your enthusiasm and willingness to listen to feedback and continously improving the library! Great work!

@og2t
Copy link

og2t commented Jun 3, 2019

I very much agree with @knavu2's proposal, thanks for considering that approach.

Do you have any timelines when this API proposal will be available to use?

@evillemez
Copy link
Contributor

@msach22 Thanks for sketching something out and requesting feedback! Generally I agree with the recommendation here from @knavu2 . Having one component that can track all of the subscribers in the session, and accept a render prop for configuring/rendering each one, would probably be a little easier to manage.

@ASerga
Copy link

ASerga commented Jun 19, 2019

@msach22, do you have any timelines when this API proposal will be available to use?

@og2t
Copy link

og2t commented Jun 24, 2019

I've created a PR #306 that will allow to render OTSubscriverView views individually.

@Tracklous
Copy link

@og2t waiting for your PR to get merged so that I can use, That is the only blocker I'm facing right now. I really appreciate your efforts :) :)

@enricop89 enricop89 mentioned this issue Jul 2, 2019
@enricop89
Copy link
Contributor

Version 0.11.2 published on NPM 🎉

@ggoldens
Copy link
Contributor

Closing this issue since it's already released.

@Shivani12345
Copy link

can we add multiple publisher or create multiple host just like multiple subscriber added here with react native?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants