Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix memory leak issue with UseEffect #1506

Merged
merged 3 commits into from
Feb 18, 2020
Merged

Fix memory leak issue with UseEffect #1506

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b58f4c7
Fix memory leak issue with `UseEffect`
larrylin28 Jan 21, 2020
eb79ce3
Drop this comment for now.
timdorr Jan 21, 2020
54c65df
Apply suggestions on parameters naming
larrylin28 Jan 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
244 changes: 148 additions & 96 deletions src/components/connectAdvanced.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,127 @@ function storeStateUpdatesReducer(state, action) {
return [action.payload, updateCount + 1]
}

function useIsomorphicLayoutEffectWithArgs(create, deps, args) {
timdorr marked this conversation as resolved.
Show resolved Hide resolved
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(
Expand Down Expand Up @@ -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.
Expand Down