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

fix: toast not appearing when toast.* called from useEffect hook #251

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jluxenberg
Copy link

@jluxenberg jluxenberg commented Dec 13, 2022

Issues

  • After calling a toast.* function from within a useEffect hook, the toast does not appear (see the new test case in toast.test.tsx for an example).
  • Calling a toast.* function before rendering the <Toaster> component, then rendering the <Toaster> component, would cause the toast to not appear or to appear "stacked up" on top of each other.

Cause

The useStore hook (used by Toaster) calls "useState" to get the current state of the global memoryState, and then calls "useEffect" to set up a subscription to the global memoryState.

The bug is caused when a change to the global memoryState happens after "useState" is called, but before the "useEffect" callback is run. This can happen e.g. if we call a toast.* function from within another component's useEffect hook.

Solution

Use the useSyncExternalStore hook instead of useState and useEffect; it handles these cases correctly.

I've added a dependency on the use-sync-external-store shim so that React version below 18 are still supported.

Related Github issues

#101
#57
#142
#253

## Issue
After calling a toast.* function from within a useEffect hook, the toast
does not appear (see the new test case in toast.test.tsx for an
example).

## Cause
The `useStore` hook (used by `Toaster`) calls "useState" to get the
current state of the global `memoryState`, and then calls "useEffect" to
set up a subscription to the global `memoryState`.

The bug is caused when a change to the global `memoryState` happens
after "useState" is called, but before the "useEffect" callback is run.
This can happen e.g. if we call a toast.* function from within another
component's useEffect hook.

## Solution
To setup the global `memoryState` subscription, we use the
"useLayoutEffect" hook instead. This hook runs the effect synchronously,
so no change to the global `memoryState` can happen before the effect is
run.

## Alternatives
`useSyncExternalStore` is a new hook that is designed to solve this
problem and is built into React; we could use this hook instead:
https://reactjs.org/docs/hooks-reference.html#usesyncexternalstore
@vercel
Copy link

vercel bot commented Dec 13, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-hot-toast ❌ Failed (Inspect) May 4, 2023 4:48pm

-- Issue
Calling a toast.* function before rendering the `<Toaster>` component,
then rendering the `<Toaster>` component, would cause the toast to not
appear or to appear "stacked up" on top of each other.

-- Cause
The `useStore` hook was using `useState` and `useEffect` to subscribe
to the global `memoryState`; unfortunately, in various cases it was
possible for change to occur to `memoryState` before the `useEffect`
callback was called. Since the `useEffect` callback hadn't run, those
changes didn't cause a re-render of the `<Toaster>` component.

-- Solution
Use the `useSyncExternalStore` hook instead of `useState` and
`useEffect`; it handles these cases correctly.

I've added a dependency on the `use-sync-external-store` shim so that
React version below 18 are still supported.

-- Related
timolins#253
@cmmartin
Copy link

I am also experiencing this bug. toast called inside useEffect do not show. or show sometime later which is not desired

@jluxenberg
Copy link
Author

@timolins Any chance of getting this merged? I just merged main into it so should be good to go. Let me know if there's anything I can do to help!

@brandanking-decently
Copy link

Hey, has anything changed with getting this fixed or merged into main. This problem still seems to be hanging around. Thanks

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

Successfully merging this pull request may close these issues.

None yet

3 participants