Wrap exported components with React.forwardRef()

[rileyjshaw]

Feature request

It would be useful to wrap DailyAudio, DailyAudioTrack, and DailyVideo in React.forwardRef().

Why you need this

I’d love to use <DailyAudioTrack />, but it’s useful to have a reference to the DOM node for HTMLMediaElements. For instance, it’s not possible to set volume under the current implementation, since you’d need access to audioEl.current.volume.

Alternatives you've considered

I think it’s most useful to simply accept a ref and join it to the internal ref with something like useMergeRefs¹. That way, the user can use the ref however they need.

But if that’s not to your taste, you could also define specific methods with useImperativeHandle.

It could also be nice to accept declarative props (like volume) and handle all of the effects internally, but it’s less flexible and more work on your end.

Additional context

I also think it would be useful to accept arbitrary props and spread them onto the returned element, like so:

export default React.forwardRefs(function DailyAudioTrack ({
  onPlayFailed,
  sessionId,
  type = 'audio',
  ...rest,
}, externalRef) {
  const audioEl = useRef(null);

  // ...

  const ref = useMergedRef(audioEl, externalRef);
  return (
    <audio
      autoPlay
      data-session-id={sessionId}
      data-audio-type={type}
      playsInline
      ref={ref}
      { ...rest }
    />
  );
});

That way, you can pass in arbitrary properties like muted.

---

Footnotes

1. Reference implementation:

   // Takes multiple refs and merges them into a single ref so you can attach
   // multiple refs to the same element.
   export function useMergedRef(refs) {
     const filteredRefs = refs.filter(Boolean);
     const mergedRef = useCallback(el => {
       filteredRefs.forEach(ref => {
         if (typeof ref === 'function') {
           ref(el);
         } else {
           ref.current = el;
         }
       });
     }, filteredRefs);
     return mergedRef;
   }
   

   ↩

[Regaddi]

Thanks @rileyjshaw, this is an excellent feature request and your reasoning is very understandable.

I'll have a look into this and get back to you with updates.

Cheers,
Christian

[Regaddi]

@rileyjshaw I have a question for you about DailyAudio: what would you expect a forwarded ref to point to? Would that be the complete list HTMLAudioElement[] of rendered audio tracks?

[rileyjshaw]

@Regaddi great question! I hadn’t considered that. Here are some quick thoughts. I was thinking through the problem while typing this, so Option 4 is my favourite. Feel free to skim the rest 😄

Option 1: Do nothing

The use case for forwarding refs to DailyAudioTrack and DailyVideo are more directly evident. I think it would be fine to say “if you need convenience, go for DailyAudio. If you need configurability, go for DailyAudioTrack”.

…but it would be really nice if DailyAudio was also configurable, so on to:

Option 2: Define a set of acceptable props for both DailyAudio and DailyAudioTrack

Quoted from above:

you could also define specific methods with useImperativeHandle

It could also be nice to accept declarative props (like volume) and handle all of the effects internally, but it’s less flexible and more work on your end.

Since DailyAudio already uses DailyAudioTrack behind the scenes, you could use a similar signature for both components. Since it will take some work to define the signature anyway, hopefully props can be used for most configuration, and hide some of the imperative implementation required to interface with the underlying <audio> element.

This sounds like a lot of work, but I think a volume prop is probably the thing most people will actually want to use. You could forget other configuration options for now (and skip useImperativeHandle), and revisit it later if anyone creates a new issue.

Option 3: Return a list of HTMLAudioElement[] tracks

This might be hard to document, as it requires the library consumer to understand a bit about how the underlying component is put together. But it could be a useful “power user” feature, and allows for a lot of flexibility!

Option 4: Combine options 2 and 3

I think this is my favourite option of the bunch.

• Create a declarative volume prop on DailyAudioTrack that works like this:

// Example stub:
const AudioExample = forwardRef(({ volume = 1, ...rest }, ref) => {
	const audioEl = useRef(null);

	// When volume changes, update the audio element’s volume.
	useEffect(() => {
		if (audioEl.current) {
			audioEl.current.volume = volume;
		}
	}, [volume]);

	// Initialize the audio element with the correct values.
	const audioCallback = useCallback(
		el => {
			if (el) {
				el.volume = volume;
			}
		},
		[volume]
	);

	const audioRef = useMergedRef([audioEl, audioCallback, ref]);

	return <audio ref={audioRef} autoPlay playsInline {...rest} />;
});

• Allow additional arbitrary props to be spread onto the <audio> or <video> elements, in case someone wants to pass in a style object, overwrite playsInline, etc. (shown above)
• Leave the rest of the wiring up to the library consumer by adding forwardRef to DailyAudioTrack (shown above) and exposing a list of refs from DailyAudio (discussed under Option 3).
• If users really want other convenience props added in the future, or extra methods using useImperativeHandle, those can be added in the future. But my instinct is that volume, style, and a forwarded ref will cover almost all use cases.

[Regaddi]

Thanks a lot for the detailed input, @rileyjshaw!

I really appreciate your thinking here, it's invaluable!

I'd like to propose another, slightly adjusted, option, based on your options 2, 3 and 4:

Instead of applying a plain ref to DailyAudio, which would eventually hold a reference to all rendered <audio> elements (as proposed in option 3), I'd like to use that passed ref to setup an imperative handle with a few methods to query for the exact <audio> elements a developer would be interested in:

interface DailyAudioHandle {
  getAllAudio(): HTMLAudioElement[];
  getActiveSpeakerAudio(): HTMLAudioElement;
  getScreenAudio(): HTMLAudioElement[];
  getAudioBySessionId(id: string): HTMLAudioElement;
  getScreenAudioBySessionId(id: string): HTMLAudioElement;
}

export const DailyAudio = forwardRef<DailyAudioHandle, Props>((props, ref) => {
  const containerRef = useRef<HTMLDivElement>(null);

  useImperativeHandle(ref, () => ({
      getAllAudio: () => containerRef.current?.querySelectorAll('audio'),
      getActiveSpeakerAudio: () =>
        containerRef.current?.querySelector(
          `audio[data-session-id="${activeSpeakerId}"][data-audio-type="audio"]`
        ),
      getScreenAudio: () =>
        containerRef.current?.querySelectorAll(
          'audio[data-audio-type="screenAudio"]'
        ),
      getAudioBySessionId: (id: string) =>
        containerRef.current?.querySelector(
          `audio[data-session-id="${id}"][data-audio-type="audio"]`
        ),
      getScreenAudioBySessionId: (id: string) =>
        containerRef.current?.querySelector(
          `audio[data-session-id="${id}"][data-audio-type="screenAudio"]`
        ),
    }));
  
  return (
    <div ref={containerRef}>
      {/* rendered audio tracks *}
    </div>
  );
});

Rationale:

• Returning a single HTMLAudioElement[] list means that developers have to manually filter for the desired <audio> element based on the data- attributes we render. Providing methods which already contain this filtering should result in less maintenance effort when updating to newer versions of @daily-co/daily-react.
• Because the actual selectors stay contained within DailyAudio we can add tests for those, making sure that we don't accidentally break the imperative handle in the future.
• I like the explicit nature of imperative handles in combination with strict types. It makes documentation a little easier and makes use of a known concept in React.

Let me know what you think, otherwise I'll probably go ahead with this approach ☺️

[rileyjshaw]

@Regaddi this sounds so useful!! Great idea 😄

[Regaddi]

This change has shipped with daily-react 0.7.0 today!