Subscribe to state changes earlier, with useLayoutEffect
#624
Conversation
so that `useEffect` calls can emit toasts on the initial render
`useEffect` won't run the callback on the server, so it should always be there?
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| @@ -1,5 +1,7 @@ | |||
| import React from 'react'; | |||
|
|
|||
| export const safeUseLayoutEffect = typeof window !== 'undefined' ? React.useLayoutEffect : () => {}; | |||
There was a problem hiding this comment.
neither useEffect or useLayoutEffect will run on the server, but useLayoutEffect will log a warning on the server, hence this
| React.useEffect(() => { | ||
| if (removed) { | ||
| const timeout = setTimeout(() => { | ||
| removeToastRef.current(toast); | ||
| }, TIME_BEFORE_UNMOUNT); | ||
| return () => clearTimeout(timeout); | ||
| } | ||
| }, [removed]); |
There was a problem hiding this comment.
moved here so that the timer is cleared if component torn down
| } | ||
| } | ||
|
|
||
| if (typeof window === 'undefined') return; |
There was a problem hiding this comment.
shouldn't be needed given inside a useEffect, which won't run on the server?
|
Hey @emilkowalski any thoughts on this? |
There was a problem hiding this comment.
Pull request overview
This PR enables toast notifications to work when triggered from useLayoutEffect by subscribing to state changes earlier in the component lifecycle. It also includes fixes for race conditions that could occur during component unmounting.
- Introduces
safeUseLayoutEffectto subscribe to toast state changes earlier in the render cycle - Refactors the Toast component's removal logic to use a ref pattern avoiding stale closures
- Adds race condition guards using a
tornDownflag to prevent state updates after unmounting
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/hooks.tsx | Adds safeUseLayoutEffect utility that conditionally uses useLayoutEffect or a fallback based on environment |
| src/index.tsx | Updates subscription logic to use safeUseLayoutEffect, refactors timeout handling with refs, adds race condition guards, and removes SSR window check |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,5 +1,7 @@ | |||
| import React from 'react'; | |||
|
|
|||
| export const safeUseLayoutEffect = typeof window !== 'undefined' ? React.useLayoutEffect : () => {}; | |||
There was a problem hiding this comment.
The safeUseLayoutEffect implementation should use React.useEffect as the fallback for server-side rendering, not an empty function. The current implementation returns an empty function that doesn't run at all, which means the subscription logic will never execute on the server and cleanup won't happen. This should be React.useEffect instead.
| export const safeUseLayoutEffect = typeof window !== 'undefined' ? React.useLayoutEffect : () => {}; | |
| export const safeUseLayoutEffect = typeof window !== 'undefined' ? React.useLayoutEffect : React.useEffect; |
| tornDown = true; | ||
| tearDown(); | ||
| }; | ||
| }, [toasts]); |
There was a problem hiding this comment.
The dependency array includes toasts, which will cause the subscription to be recreated every time the toasts array changes. This defeats the purpose of using a subscription pattern and could lead to missed updates or duplicate subscriptions. The dependency array should be empty [] to ensure the subscription is only set up once on mount, similar to the useSonner hook at line 603.
| }, [toasts]); | |
| }, []); |
1 participant
Thanks for the great library!
I wanted to show an error toast using
toast.error()from inside auseLayoutEffect, but it didn't work.This fixes that 🎉
So now providing the
useLayoutEffectis from a component rendered after<Sonner />it should work, and this also means that usinguseEffectanywhere should always work regardless of whether<Sonner />is before or after the component using the hook.Related #168
I also spotted a few race conditions so added some fixes for those in here too
I tried to run the tests locally but
npm run devdoesn't seem to start the web server for some reason, not sure why