Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
GiffE opened this issue Jun 20, 2024 · 1 comment
Open

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

GiffE opened this issue Jun 20, 2024 · 1 comment

Comments

@GiffE
Copy link

GiffE commented Jun 20, 2024

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:
#3593

@bouzidanas
Copy link
Contributor

bouzidanas commented Aug 23, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants