Wrap all alert payloads in useMemo to avoid infinite re-rendering (#182)

* Wrap all alert payloads in useMemo to avoid infinite re-rendering

This manifested in production as a browser freeze for any
user who saw the 15%-off-to-upgrade message.

TNL-7400

* fixup! meant to say identity, not equality
This commit is contained in:
Kyle McCormick
2020-08-12 17:12:31 -04:00
committed by GitHub
parent cc7142e5c1
commit bc76adf8eb
6 changed files with 37 additions and 25 deletions

View File

@@ -8,6 +8,17 @@ export function useAlert(isVisible, {
const { add, remove } = useContext(UserMessagesContext);
const [alertId, setAlertId] = useState(null);
// Please note:
// The deps list [isVisible, code, ... etc.] in this `useEffect` call prevents the
// effect from running if none of deps have changed. However, "changed" for objects is
// defined in terms of identity; thus, if you provide a payload that is *seemingly* equal
// to the previous one but *actually* a different object, then this effect will run.
// If you are particularly unlucky, this will cause an infinite re-render loop.
// This manifested itself in TNL-7400.
// We hope to address the underlying issue in TNL-7418.
// In the mean time, you may follow the pattern that `useAccessExpirationAlert`
// establishes: memoize the payload so that the exact same object is used if the
// payload has not changed.
useEffect(() => {
if (isVisible && alertId === null) {
setAlertId(add({