daily-co / daily-js

https://docs.daily.co/reference/daily-js
BSD 2-Clause "Simplified" License
103 stars 33 forks source link

Cleaning up the iFrame in React useEffect is impossible #229

Closed ahkhanjani closed 9 months ago

ahkhanjani commented 12 months ago

So I've been trying everything that came to mind but all of them have failed. I asked a similar thing before in #205 and the answer by @Regaddi kinda worked for the time being. But that's just a hack and randomly causes client-side exceptions.

The appropriate approach has to be calling .destroy() inside the useEffect cleanup function. Here's the code that I'm trying to use:

function Call() {
  const [callFrame, setCallFrame] = useState<DailyCall | null>(null);
  const callRef = useRef<HTMLDivElement>(null);

  useEffect(() => {
    const domCallFrame = callRef.current;
    if (!domCallFrame) return;

    const newCallFrame = DailyIframe.createFrame(callRef.current, CALL_OPTIONS);

    void newCallFrame.join({ url: roomUrl, token: meetingToken });
    setCallFrame(newCallFrame);

    newCallFrame.on("left-meeting", () => {
      void newCallFrame.destroy();
      setCallFrame(null);
      void router.push("/dashboard/calendar");
    });

    return () => {
      void newCallFrame.destroy();
      setCallFrame(null);
    };
  }, []);

  return <div ref={callRef} />;
}

Using callRef.destroy() instead of newCallRef.destroy(), removing the iFrame manually using callFrame.iframe().remove(), etc. all seem to result in this error: Error: Duplicate DailyIframe instances are not allowed.

I suspect the problem with this error and the unknown client-side exception I frequently get has something to do with the inability to cleanup the existing call object and the iFrame not being removed. The reason it fails is that callFrame is null in the current render and we can't call destroy on it until the next render. Also .destroy() is an async function so it probably doesn't destroy anything until after React has rerendered the component, resulting in the duplicate iFrame.

There are surprisingly few examples of Daily prebuilt with React on the internet and the existing ones don't mention this. Here's an example. I assume they didn't have the React strict mode on.

I would really appreciate a simple practical working repo or example.

Regaddi commented 12 months ago

Hey @ahkhanjani,

I feel your frustration with this. We've pretty much solved this problem in daily-react, but there we only create callObject instances. Perhaps it's worth providing a hook that allows to create callFrame instances, too, with a correct cleanup mechanism in place.

Something like:

const callFrame = useCallFrame({
  // frame properties
});

The underlying code for this is already accessible in daily-react's DailyProvider component. Extracting that into reusable hooks for callFrame and callObject creation shouldn't take a lot of work. I can take a look at that today.

Best Christian

ahkhanjani commented 12 months ago

Hi @Regaddi,

You have my thanks. That will solve a lot of headaches.

While the idea of built-in hooks is in process, I think I should mention the sole use of the callFrame state in my code. It is to trigger callFrame.setTheme() on website's theme change inside another useEffect. I'm wondering if we can have access to the call frame events via the helper hooks so that we won't need to manually manage the state ourselves like this.

ahkhanjani commented 10 months ago

Hi @Regaddi, Any chances we can have this in the upcoming version?

Regaddi commented 10 months ago

Hey @ahkhanjani,

I'm sorry for the delay here. We've been successfully running with the new hooks internally for a couple of weeks now! I'm actively working on a new release version for Daily React and will try to push it today, or Monday at latest.

The hook itself is already available on the repository's main branch, in case you want to check the implementation and API already: useCallFrame()

Based on your original comment, you'd probably use it like something like this:

function Call() {
  const callRef = useRef<HTMLDivElement>(null);
  const callFrame = useCallFrame({
    parentEl: callRef.current,
    options: CALL_OPTIONS,
  });

  useEffect(() => {
    if (!callFrame) return;

    void callFrame.join({ url: roomUrl, token: meetingToken });

    callFrame.on("left-meeting", () => {
      void callFrame.destroy();
      void router.push("/dashboard/calendar");
    });
  }, [callFrame]);

  return <div ref={callRef} />;
}

useCallFrame also accepts an optional shouldCreateInstance(): boolean callback to have more fine-grained control when a call instance should be created or not. In your example, you might not want to create a new instance, when the meeting is left as an example, but since you're navigating away from the page, it might be good as it is.

I would also recommend wrapping your call with DailyProvider and pass it the callFrame:

// Yes, this works! callFrame and callObject share the same type `DailyCall`
<DailyProvider callObject={callFrame}>…</DailyProvider>

Then you'd be able to setup the left-meeting event listener with useDailyEvent and leverage other goodies of the library.

Keep an eye on Daily React's releases page!

Best Christian

ahkhanjani commented 10 months ago

Thank you for your response @Regaddi. This looks great!

Could we please have an example project using this on release?

Regaddi commented 9 months ago

Daily React 0.17.0 has been released. The library exports new hooks useCallFrame() and useCallObject(), which solve this issue.

Instead of creating custom call instances inside of useEffect, developers can now use these hooks.

Could we please have an example project using this on release?

@ahkhanjani I'm curious what kind of example would be most helpful for a broader audience. In essence you'll replace your custom useEffect to create call instances with useCallFrame or useCallObject.

ahkhanjani commented 9 months ago

Thank you @Regaddi. I have yet to try this but it looks very straightforward. I will open an issue if I encounter any problems or questions.

I'm curious what kind of example would be most helpful for a broader audience. In essence you'll replace your custom useEffect to create call instances with useCallFrame or useCallObject.

The documentation looks great so there's probably no need for an example anymore.

saintpepsicola commented 3 months ago

@Regaddi Can we please have this on React Native? My team rejected using Daily because of this big annoyance.