fix for #109 #110
fix for #109 #110
Conversation
parisholley
force-pushed
the
fast-refresh
branch
from
158c964
to
67fa326
Compare
| @@ -530,7 +530,24 @@ export function useState<S>( | |||
| RootPath, | |||
| () => setValue({ state: value.state }), | |||
| value.state); | |||
| React.useEffect(() => () => value.state.destroy(), []); | |||
| const renders = React.useRef(0); | |||
There was a problem hiding this comment.
Nice clever fix. Yes, it will impact the performance, unfortunately. However, there is a way around it. Define module-global isDevelopment boolean flag and execute the proposed code only when it is true, otherwise keep the original one under release/production mode. And please let me know if tests are passing with it (I understand they should be). And I will merge after (if you complete it within next 12 hours, I will merge it today, otherwise only after New Year holidays :) )
|
|
||
| return () => { | ||
| if (capture !== renders.current) { | ||
| // do not destroy state if a third party (eg: react-fast-refresh) has restored it on re-render |
There was a problem hiding this comment.
Please add link in comments to this page: apollographql/apollo-client#5870 (comment)
very useful
|
I will integrate it and finalize before leaving today. Thanks |
|
thanks for getting this merged so quickly! I finally got tired of hard refreshing the page all the time... :) as a note, i realized after the pull that using useMemo technically isn't a guarantee, so i used a locking mechanism instead: i'm not sure how likely the original implementation would be a problem |
|
I do not think your lock implementation is right, because it essentially blocks state destruction after any rerender, hotreload or after setState call. |
|
why do you say that? the useRef is persisted between rerender/reload/setstate, and because i removed the [] on the useEffect, it is effectively run after every render, removing the lock. i can't think of a circumstance in which the lock will have contention outside of hotreload, unless there is some behavior that can cause a render to happen multiple times before the useEffect is invoked. |
|
I missed that useEffect goes on every rerender. Now I do not understand what sequence of events would cause the lock to be true on effect destruction callback? |
|
Is the following sequence happens on hot reload?
|
|
If you are 100% sure it is the case and would like the improved version, please submit pull request again. I doubt I will have time today to merge it, but will be able to come back to it after the holidays break... |
|
I have integrated your latest proposal. Released in 3.0.3. Let me know how it goes. |
|
works well 👍 |
see the discussion here on how a similar issue affects all react libs for the most part:
apollographql/apollo-client#5870 (comment)
apollographql/apollo-client#6661
effectively, under normal circumstances, useEffect should only be invoked whenever values change, however when react-fast-refresh is active, the dependencies for useEffect are ignored and it is invoked again, allowing us to check if things are out-of-sync.
i'm unsure if this approach will affect benchmarks or not.