From 9f078f043fa86e96262bc02174ef2141455c87ac Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 4 May 2021 17:19:20 -0400 Subject: [PATCH 1/3] Recheck rv() against previous value in useIsomorphicEffect. While rereading the code @jcreighton added in #7652, I remembered it's safe to call rv() in a useEffect callback (without useReactiveVar), which allows us to compare the latest variable value to the previous value before deciding whether to call rv.onNextChange(setValue) in the effect callback function. If the values already disagree, we can skip listening and just call setValue right away. In React , the two initial rv() values generated prior to the useIsomorphicEffect callback should always be the same, unless the reactive variable somehow gets updated in between those renders, which I don't think is possible, since React invokes those renders sychronously, one after the other: https://github.com/facebook/react/blob/0e100ed00fb52cfd107db1d1081ef18fe4b9167f/packages/react-reconciler/src/ReactUpdateQueue.new.js#L391-L399 The key difference between this approach and calling rv.onNextChange synchronously in render is that (in this new approach) we can ignore the extra value without doing anything to clean up its resources, whereas with rv.onNextChange we were also responsible for calling the extra cancel function produced by the extra rv.onNextChange call. --- src/react/hooks/useReactiveVar.ts | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/react/hooks/useReactiveVar.ts b/src/react/hooks/useReactiveVar.ts index 3e3e79e7dc5..891fcd163ab 100644 --- a/src/react/hooks/useReactiveVar.ts +++ b/src/react/hooks/useReactiveVar.ts @@ -1,7 +1,6 @@ import { useEffect, useLayoutEffect, useState } from 'react'; import { ReactiveVar } from '../../core'; - const isBrowser = typeof window !== 'undefined' && typeof window.document !== 'undefined' && typeof window.document.createElement !== 'undefined'; @@ -10,21 +9,24 @@ const useIsomorphicEffect = isBrowser ? useLayoutEffect : useEffect; export function useReactiveVar(rv: ReactiveVar): T { const value = rv(); + // We don't actually care what useState thinks the value of the variable // is, so we take only the update function from the returned array. const [, setValue] = useState(value); + // We subscribe to variable updates on initial mount and when the value has // changed. This avoids a subtle bug in React.StrictMode where multiple listeners // are added, leading to inconsistent updates. - useIsomorphicEffect(() => rv.onNextChange(setValue), [value]); - // Once the component is unmounted, ignore future updates. Note that the - // above useEffect function returns a mute function without calling it, - // allowing it to be called when the component unmounts. This is - // equivalent to the following, but shorter: - // useEffect(() => { - // const mute = rv.onNextChange(setValue); - // return () => mute(); - // }, [value]) + useIsomorphicEffect(() => { + const probablySameValue = rv(); + if (value !== probablySameValue) { + // If the value of rv has already changed, we don't need to listen for the + // next change, because we can report this change immediately. + setValue(probablySameValue); + } else { + return rv.onNextChange(setValue); + } + }, [value]); // We check the variable's value in this useEffect and schedule an update if // the value has changed. This check occurs once, on the initial render, to avoid From 5ec07e5dd5b65a97ae93badc4020e103962469bb Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 4 May 2021 17:50:56 -0400 Subject: [PATCH 2/3] Unwind remaining workarounds in useReactiveVar. --- src/react/hooks/useReactiveVar.ts | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/src/react/hooks/useReactiveVar.ts b/src/react/hooks/useReactiveVar.ts index 891fcd163ab..d197895dc79 100644 --- a/src/react/hooks/useReactiveVar.ts +++ b/src/react/hooks/useReactiveVar.ts @@ -1,12 +1,6 @@ -import { useEffect, useLayoutEffect, useState } from 'react'; +import { useEffect, useState } from 'react'; import { ReactiveVar } from '../../core'; -const isBrowser = typeof window !== 'undefined' && - typeof window.document !== 'undefined' && - typeof window.document.createElement !== 'undefined'; - -const useIsomorphicEffect = isBrowser ? useLayoutEffect : useEffect; - export function useReactiveVar(rv: ReactiveVar): T { const value = rv(); @@ -17,7 +11,7 @@ export function useReactiveVar(rv: ReactiveVar): T { // We subscribe to variable updates on initial mount and when the value has // changed. This avoids a subtle bug in React.StrictMode where multiple listeners // are added, leading to inconsistent updates. - useIsomorphicEffect(() => { + useEffect(() => { const probablySameValue = rv(); if (value !== probablySameValue) { // If the value of rv has already changed, we don't need to listen for the @@ -28,12 +22,5 @@ export function useReactiveVar(rv: ReactiveVar): T { } }, [value]); - // We check the variable's value in this useEffect and schedule an update if - // the value has changed. This check occurs once, on the initial render, to avoid - // a useEffect higher in the component tree changing a variable's value - // before the above useEffect can set the onNextChange handler. Note that React - // will not schedule an update if setState is called with the same value as before. - useEffect(() => setValue(rv()), []); - return value; } From 9555675c25fc888c9f21a549c8544b6672a3fc44 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 4 May 2021 19:56:59 -0400 Subject: [PATCH 3/3] Mention PRs #8126 and #8135 in CHANGELOG.md. --- CHANGELOG.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ec89c98d6d8..004201e2b55 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,13 @@ +## Apollo Client 3.3.17 (not yet released) + +### Bug fixes + +- Prevent warnings about `useLayoutEffect` when using `useReactiveVar` during React server rendering.
+ [@brainkim](https://github.com/brainkim) in [#8126](https://github.com/apollographql/apollo-client/pull/8126) + +- Make `useReactiveVar(rv)` recheck the latest `rv()` value in its `useEffect` callback, and immediately update state if the value has already changed, rather than calling `rv.onNextChange(setValue)` to listen for future changes.
+ [@benjamn](https://github.com/benjamn) in [#8135](https://github.com/apollographql/apollo-client/pull/8135) + ## Apollo Client 3.3.16 ### Bug fixes