Fix memory leak issue with UseEffect

There's a memory leak in `react-redux`'s usage of UseEffect, here's the detail:
In the last useIsomorphicLayoutEffect usage in connectAdvanced.js, it returns a funcion, `unsubscribeWrapper`, which will be retained by React as `destroy` function of the effect. Since this `useEffect` only subscribes to `store`, `subscription`, `childPropsSelector`, which in most case won't change when store state updates. So this `useEffect` is never called again in following updates of `connected` component. So the effect in the `connected` component will always keep a reference to the `unsubscribeWrapper` created in first call. But this will lead to a memory leak in modern JS VM.
In modern JS VM such as V8(Chrome), the instance of function `unsubscribeWrapper` will retain a closure for context when it's created. Which means, all local variables in first call of each `connected` component will be retained by this instance of `unsubscribeWrapper`, even though they are not used by it at all. In this case, the context includes `actualChildProps`. Which means, every connected component, will in the end retain 2 copy of its props in the memory, one as it's current prop, another is the first version of its props.
It can be huge impact of memory if a connected component has props retaining a reference to big chunk of data in store state that can be fully updated to another version(e.g. data parsed from cache, network repsonse, etc). It will end up always retaining 2 copy of that data in memory, or more if there're more other `connected` compoents.
A better JS VM should optimize to not include unused variable in the closure, but as I tested in V8 and Hermes, they both doesn't have such optimisation to avoid this case.

This can be easy reproduced:
1. Have a connected component, reference one object in the store state.
2. Update the store state(add a version maker in the object to help identify the issue)
3. Use Chrome dev tools to take a heap snapshot.
4. Search for the object in the heap snapshot, you will find 2 version of it in the heap, one retained by connected wrapped component's props, one retained by a `destroy` in lastEffect of conneted compoents.

By communicating with React community, a good solution suggested is to lift `useEffect` outside of the hook component in such kind of case.
And this is how this PR solve the problem.

Author: larrylin28

src/components/connectAdvanced.js

@@ -23,6 +23,127 @@ function storeStateUpdatesReducer(state, action) {
   return [action.payload, updateCount + 1]
 }
 
+function useIsomorphicLayoutEffectWithArgs(create, deps, args) {
+  useIsomorphicLayoutEffect(() => create(...args), deps)
+}
+
+function captureWrapperProps(
+  lastWrapperProps,
+  lastChildProps,
+  renderIsScheduled,
+  wrapperProps,
+  actualChildProps,
+  childPropsFromStoreUpdate,
+  notifyNestedSubs
+) {
+  // We want to capture the wrapper props and child props we used for later comparisons
+  lastWrapperProps.current = wrapperProps
+  lastChildProps.current = actualChildProps
+  renderIsScheduled.current = false
+
+  // If the render was from a store update, clear out that reference and cascade the subscriber update
+  if (childPropsFromStoreUpdate.current) {
+    childPropsFromStoreUpdate.current = null
+    notifyNestedSubs()
+  }
+}
+
+function subscribeUpdates(
+  shouldHandleStateChanges,
+  store,
+  subscription,
+  childPropsSelector,
+  lastWrapperProps,
+  lastChildProps,
+  renderIsScheduled,
+  childPropsFromStoreUpdate,
+  notifyNestedSubs,
+  forceComponentUpdateDispatch
+) {
+  // If we're not subscribed to the store, nothing to do here
+  if (!shouldHandleStateChanges) return
+
+  // Capture values for checking if and when this component unmounts
+  let didUnsubscribe = false
+  let lastThrownError = null
+
+  // We'll run this callback every time a store subscription update propagates to this component
+  const checkForUpdates = () => {
+    if (didUnsubscribe) {
+      // Don't run stale listeners.
+      // Redux doesn't guarantee unsubscriptions happen until next dispatch.
+      return
+    }
+
+    const latestStoreState = store.getState()
+
+    let newChildProps, error
+    try {
+      // Actually run the selector with the most recent store state and wrapper props
+      // to determine what the child props should be
+      newChildProps = childPropsSelector(
+        latestStoreState,
+        lastWrapperProps.current
+      )
+    } catch (e) {
+      error = e
+      lastThrownError = e
+    }
+
+    if (!error) {
+      lastThrownError = null
+    }
+
+    // If the child props haven't changed, nothing to do here - cascade the subscription update
+    if (newChildProps === lastChildProps.current) {
+      if (!renderIsScheduled.current) {
+        notifyNestedSubs()
+      }
+    } else {
+      // Save references to the new child props.  Note that we track the "child props from store update"
+      // as a ref instead of a useState/useReducer because we need a way to determine if that value has
+      // been processed.  If this went into useState/useReducer, we couldn't clear out the value without
+      // forcing another re-render, which we don't want.
+      lastChildProps.current = newChildProps
+      childPropsFromStoreUpdate.current = newChildProps
+      renderIsScheduled.current = true
+
+      // If the child props _did_ change (or we caught an error), this wrapper component needs to re-render
+      forceComponentUpdateDispatch({
+        type: 'STORE_UPDATED',
+        payload: {
+          error
+        }
+      })
+    }
+  }
+
+  // Actually subscribe to the nearest connected ancestor (or store)
+  subscription.onStateChange = checkForUpdates
+  subscription.trySubscribe()
+
+  // Pull data from the store after first render in case the store has
+  // changed since we began.
+  checkForUpdates()
+
+  const unsubscribeWrapper = () => {
+    didUnsubscribe = true
+    subscription.tryUnsubscribe()
+    subscription.onStateChange = null
+
+    if (lastThrownError) {
+      // It's possible that we caught an error due to a bad mapState function, but the
+      // parent re-rendered without this component and we're about to unmount.
+      // This shouldn't happen as long as we do top-down subscriptions correctly, but
+      // if we ever do those wrong, this throw will surface the error in our tests.
+      // In that case, throw the error from here so it doesn't get lost.
+      throw lastThrownError
+    }
+  }
+
+  return unsubscribeWrapper
+}
+
 const initStateUpdates = () => [null, 0]
 
 export default function connectAdvanced(
@@ -278,107 +399,38 @@ export default function connectAdvanced(
         return childPropsSelector(store.getState(), wrapperProps)
       }, [store, previousStateUpdateResult, wrapperProps])
 
+      // To avoid memory leak issue of function closure in useEffect, move useEffect functions out of component scope.
+
       // We need this to execute synchronously every time we re-render. However, React warns
       // about useLayoutEffect in SSR, so we try to detect environment and fall back to
       // just useEffect instead to avoid the warning, since neither will run anyway.
-      useIsomorphicLayoutEffect(() => {
-        // We want to capture the wrapper props and child props we used for later comparisons
-        lastWrapperProps.current = wrapperProps
-        lastChildProps.current = actualChildProps
-        renderIsScheduled.current = false
-
-        // If the render was from a store update, clear out that reference and cascade the subscriber update
-        if (childPropsFromStoreUpdate.current) {
-          childPropsFromStoreUpdate.current = null
-          notifyNestedSubs()
-        }
-      })
+      useIsomorphicLayoutEffectWithArgs(captureWrapperProps, undefined, [
+        lastWrapperProps,
+        lastChildProps,
+        renderIsScheduled,
+        wrapperProps,
+        actualChildProps,
+        childPropsFromStoreUpdate,
+        notifyNestedSubs
+      ])
 
       // Our re-subscribe logic only runs when the store/subscription setup changes
-      useIsomorphicLayoutEffect(() => {
-        // If we're not subscribed to the store, nothing to do here
-        if (!shouldHandleStateChanges) return
-
-        // Capture values for checking if and when this component unmounts
-        let didUnsubscribe = false
-        let lastThrownError = null
-
-        // We'll run this callback every time a store subscription update propagates to this component
-        const checkForUpdates = () => {
-          if (didUnsubscribe) {
-            // Don't run stale listeners.
-            // Redux doesn't guarantee unsubscriptions happen until next dispatch.
-            return
-          }
-
-          const latestStoreState = store.getState()
-
-          let newChildProps, error
-          try {
-            // Actually run the selector with the most recent store state and wrapper props
-            // to determine what the child props should be
-            newChildProps = childPropsSelector(
-              latestStoreState,
-              lastWrapperProps.current
-            )
-          } catch (e) {
-            error = e
-            lastThrownError = e
-          }
-
-          if (!error) {
-            lastThrownError = null
-          }
-
-          // If the child props haven't changed, nothing to do here - cascade the subscription update
-          if (newChildProps === lastChildProps.current) {
-            if (!renderIsScheduled.current) {
-              notifyNestedSubs()
-            }
-          } else {
-            // Save references to the new child props.  Note that we track the "child props from store update"
-            // as a ref instead of a useState/useReducer because we need a way to determine if that value has
-            // been processed.  If this went into useState/useReducer, we couldn't clear out the value without
-            // forcing another re-render, which we don't want.
-            lastChildProps.current = newChildProps
-            childPropsFromStoreUpdate.current = newChildProps
-            renderIsScheduled.current = true
-
-            // If the child props _did_ change (or we caught an error), this wrapper component needs to re-render
-            forceComponentUpdateDispatch({
-              type: 'STORE_UPDATED',
-              payload: {
-                error
-              }
-            })
-          }
-        }
-
-        // Actually subscribe to the nearest connected ancestor (or store)
-        subscription.onStateChange = checkForUpdates
-        subscription.trySubscribe()
-
-        // Pull data from the store after first render in case the store has
-        // changed since we began.
-        checkForUpdates()
-
-        const unsubscribeWrapper = () => {
-          didUnsubscribe = true
-          subscription.tryUnsubscribe()
-          subscription.onStateChange = null
-
-          if (lastThrownError) {
-            // It's possible that we caught an error due to a bad mapState function, but the
-            // parent re-rendered without this component and we're about to unmount.
-            // This shouldn't happen as long as we do top-down subscriptions correctly, but
-            // if we ever do those wrong, this throw will surface the error in our tests.
-            // In that case, throw the error from here so it doesn't get lost.
-            throw lastThrownError
-          }
-        }
-
-        return unsubscribeWrapper
-      }, [store, subscription, childPropsSelector])
+      useIsomorphicLayoutEffectWithArgs(
+        subscribeUpdates,
+        [store, subscription, childPropsSelector],
+        [
+          shouldHandleStateChanges,
+          store,
+          subscription,
+          childPropsSelector,
+          lastWrapperProps,
+          lastChildProps,
+          renderIsScheduled,
+          childPropsFromStoreUpdate,
+          notifyNestedSubs,
+          forceComponentUpdateDispatch
+        ]
+      )
 
       // Now that all that's done, we can finally try to actually render the child component.
       // We memoize the elements for the rendered child component as an optimization.