callthemonline / react-sip

React wrapper for jssip
https://www.npmjs.com/package/react-sip
MIT License
50 stars 46 forks source link

Guard against multiple SipProviders #22

Closed Steveb-p closed 6 years ago

Steveb-p commented 6 years ago

This change adds ID attribute to audio element when attaching to document, checks against existence of one such element during component mounting and properly removes audio element when SipProvider is unmounted.

Steveb-p commented 6 years ago

I've noticed that PR, but it was already too late and PR was submitted :) I'll gladly do the rebase.

By the way, is there any reason why multiple audio elements are forbidden? Exposing audio element would potentially allow usage of audio filters maybe as well, so I was thinking of exposing ref of created element for this reason.

kachkaev commented 6 years ago

We can discuss passing the ref down in a separate issue or a PR if you want – I'd be keen to see a usage example. Now that you give the element a fixed ID, won't that be enough for configuring filters etc.?

As of a single <audio> tag, I don't exclude that technically we can afford more than one instance (same for the mic?). That said, I can hardly image a scenario when someone would want to have two SipProviders in one app. Anyway, if there is a need, I'm open to a change!

Steveb-p commented 6 years ago

In my app I've changed the SipProvider to use new context API and changed the render function as follows:

render() {

    return (
      <React.Fragment>
        <audio ref={el => this.audioRef = el} />
        <SipContext.Provider value={{
          audioElement: this.audioRef,
          registerSip: this.registerSip,
          unregisterSip: this.unregisterSip,
          answerCall: this.answerCall,
          startCall: this.startCall,
          stopCall: this.stopCall,
        }}>
          { this.props.children }
        </SipContext.Provider>
      </React.Fragment>
    );
  }

This way there is no introduction of new id to window and audio element is available in and only in relevant context. I'm still wondering if re-rendering of audio element will be causing issues, since I haven't yet checked.

Otherwise, I would just pass the id as prop to allow using whatever id one wants, and to allow multiple SipProviders. As for use case, the one I'm going for is putting calls on hold and making new ones while the original is still pending. It might be invalid approach, but I'm relatively new to SIP protocol and related RTC.

EDIT: Properties in sample are not passed down, because I've made them part of redux state, but you get the idea.

kachkaev commented 6 years ago

Can SipContext.Provider work in parallel with the old context? Have you seen any projects that do this?

UPD: let's keep the context-related conversation in https://github.com/callthemonline/react-sip/issues/19

Steveb-p commented 6 years ago

It's just a sample. Not really relevant to <audio> element itself :) You can switch back to original <SipProvider> and it would work the same way, i.e.

render() {

    return (
      <React.Fragment>
        <audio ref={el => this.audioRef = el} />
        { this.props.children }
      </React.Fragment>
    );
  }

For it to work it would mean checking against React.createContext function presence and switching between old and new implementation, I guess.

kachkaev commented 6 years ago

@Steveb-p we'll be releasing a new version today – do you have an opportunity to rebase the existing changes? We can think of further improvements later I guess.

Steveb-p commented 6 years ago

@kachkaev rebased and changes reapplied to index.ts

kachkaev commented 6 years ago

Thanks for doing this so quickly @Steveb-p! Could you please run yarn format to make prettier happy? Otherwise Travis fails.

Steveb-p commented 6 years ago

@kachkaev done. I figure other reformatted files are supposed to be left alone, so I did ;) (some .md and package.json)

kachkaev commented 6 years ago

@Steveb-p awesome, many thanks! LGTM 🎉

kachkaev commented 6 years ago

0.8.0 is out 😉