hakimel / reveal.js

The HTML Presentation Framework
https://revealjs.com
MIT License
67.56k stars 16.62k forks source link

docs: In React -> Reveal.destroy on unmount does not fully destroy #3645

Open GiffE opened 2 months ago

GiffE commented 2 months ago

The docs suggestion on how to use Reveal with react seem to have a bug.

From: https://revealjs.com/react/

    const deckRef = useRef<Reveal.Api | null>(null); // reference to deck reveal instance

    useEffect(() => {
        // Prevents double initialization in strict mode
        if (deckRef.current) return;

        deckRef.current = new Reveal(deckDivRef.current!, {
            transition: "slide",
            // other config options
        });

        deckRef.current.initialize().then(() => {
            // good place for event handlers and plugin setups
        });

        return () => {
            try {
                if (deckRef.current) {
                    deckRef.current.destroy();
                    deckRef.current = null;
                }
            } catch (e) {
                console.warn("Reveal.js destroy call failed.");
            }
        };
    }, []);

This expressly violates the pitfall described in the React documentation: https://react.dev/learn/synchronizing-with-effects#dont-use-refs-to-prevent-effects-from-firing

This causes reveal to not unmount properly, the observable bug is that the keyboard bindings from a previous instance remain.

React docs suggest that a cancelation be used instead: https://react.dev/learn/synchronizing-with-effects#fetching-data

Writing in an abort variable seems to fix the issue along with setting the ref only after initialization.

    useEffect(() => {
        let abortInitialize = false;
        let deck = new Reveal(deckDivRef.current!, {
            transition: "slide",
            // other config options
        });

        deck.initialize().then(() => {

            if (abortInitialize) {
                deck.destroy();
                return;
            }

            // good place for event handlers and plugin setups

            deckRef.current = deck;
        });

        return () => {
            try {
                abortInitialize = true;
                if (deckRef?.current?.isReady()) {
                    deckRef.current.destroy();
                    deckRef.current = null;
                }
            } catch (e) {
                console.warn('Reveal.js destroy call failed.');
            }
        };
    }, []);

Related issue: https://github.com/hakimel/reveal.js/issues/3593

bouzidanas commented 2 weeks ago

Some thoughts:

I have yet to test your suggestion but I noticed that you try to abort the initialization inside the then block which happens after initialization. If the destroy function does not fully revert the changes to the DOM that the initialization function makes, then there will be problems that will result. The block should make sure that any deck is only ever initialized once. It would be convenient if Reveal.js only allows a deck to be initialized once perhaps similar to how plugins do it. Currently, setting the ref to null in the return acts similar to a flag that prevents re-initialization.