Fix/fix development error with react strict mode #32
Conversation
|
this works for me, thank you, I believe it should be merged |
|
Can we merge this please? I would like to have this soon otherwise will have to patch it manually |
|
tbh I think this PR makes a bunch of changes that are unrelated to the original issue |
| return () => { | ||
| _mounted.current = false | ||
| } |
There was a problem hiding this comment.
Does this useEffect need to return this? _mounted.current is not referenced or otherwise modified by this useEffect
| }: FormPersistConfig | ||
| ) => { | ||
| const watchedValues = watch() | ||
| const [localExclude] = useState<string[]>(exclude) |
There was a problem hiding this comment.
What is the reason for this useState?
By initializing useState with exclude we also lose any reactivity to the value of exclude.
| const getStorage = () => storage || window.sessionStorage | ||
|
|
||
| const clearStorage = () => getStorage().removeItem(name) | ||
| const getStorage = useCallback(() => storage || window.sessionStorage, [storage]) |
There was a problem hiding this comment.
Is the memoization of this function worth the overhead cost of useCallback?
| const clearStorage = () => getStorage().removeItem(name) | ||
| const getStorage = useCallback(() => storage || window.sessionStorage, [storage]) | ||
| const clearStorage = useCallback(() => getStorage().removeItem(name), [getStorage, name]) | ||
| const onTimeoutCallback = useCallback(() => onTimeout && onTimeout(), [onTimeout]) |
There was a problem hiding this comment.
Why extract this to another useCallback? Seemed fine as an inline call imo
| const getStorage = useCallback(() => storage || window.sessionStorage, [storage]) | ||
| const clearStorage = useCallback(() => getStorage().removeItem(name), [getStorage, name]) | ||
| const onTimeoutCallback = useCallback(() => onTimeout && onTimeout(), [onTimeout]) | ||
| const _mounted = useRef(true) |
There was a problem hiding this comment.
I would suggest the value of mounted should be initially false, since we shouldn't consider any component to be mounted until a useEffect runs
3 participants
Hello.
This fixes a bug that happens when using
react-hook-formwithdefaultValuesoption.https://beta.reactjs.org/learn/synchronizing-with-effects#how-to-handle-the-effect-firing-twice-in-development
It basically makes sure it only runs once using the good old _mounted ref.
Should fix: #18
I also added a test.