Skip to content
This repository has been archived by the owner on Nov 13, 2022. It is now read-only.

Guard against multiple SipProviders #22

Merged
merged 1 commit into from
Aug 4, 2018

Conversation

Steveb-p
Copy link
Contributor

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.

Copy link
Collaborator

@kachkaev kachkaev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this addition @Steveb-p!

The PR looks good to me, could you just rebase it on top of the latest master? We have recently switched to TypeScript, have replaced ESLint with TSLint and have also introduced Prettier (#21). Your changes should fit well on top of that, the name of the file just changes from .js to .ts.

You can run yarn format before making the commit to make sure CI does not fail when checking code style.

Once your PR is merged, @denisnikulin or I will create a new PR to release 0.8, which will include your change. A reminder to myself: don't forget about the changelog.

@Steveb-p
Copy link
Contributor Author

Steveb-p commented Aug 1, 2018

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
Copy link
Collaborator

kachkaev commented Aug 1, 2018

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
Copy link
Contributor Author

Steveb-p commented Aug 2, 2018

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
Copy link
Collaborator

kachkaev commented Aug 2, 2018

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 #19

@Steveb-p
Copy link
Contributor Author

Steveb-p commented Aug 2, 2018

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
Copy link
Collaborator

kachkaev commented Aug 4, 2018

@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
Copy link
Contributor Author

Steveb-p commented Aug 4, 2018

@kachkaev rebased and changes reapplied to index.ts

@kachkaev
Copy link
Collaborator

kachkaev commented Aug 4, 2018

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

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
Copy link
Contributor Author

Steveb-p commented Aug 4, 2018

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

@kachkaev
Copy link
Collaborator

kachkaev commented Aug 4, 2018

@Steveb-p awesome, many thanks! LGTM 🎉

@kachkaev kachkaev merged commit e16052b into callthemonline:master Aug 4, 2018
@kachkaev
Copy link
Collaborator

kachkaev commented Aug 4, 2018

0.8.0 is out 😉

@Steveb-p Steveb-p deleted the patch-1 branch October 6, 2020 16:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants