reduxjs / reselect

Selector library for Redux
MIT License
19.03k stars 671 forks source link

Fix leaking Ref in weakMapMemoize #729

Closed tomavos closed 1 month ago

tomavos commented 1 month ago

This PR contains the following changes:

Context:

The previous code could leak a WeakRef if the dereferenced value was undefined (which I think happens when the original object is forcefully garbage collected): Let lastResult be a Ref to undefined:

const lastResultValue = lastResult?.deref?.() ?? lastResult
-> const lastResultValue = lastResult.deref() ?? lastResult
-> const lastResultValue = undefined ?? lastResult`
-> const lastResultValue = lastResult (= Ref{undefined})

With this new change, we will return the new value (even when the old garbage collected value would've passed the resultEqualityCheck and would've been memoized).

I chose the instanceof check because Ref is defined above as a WeakRef if it is available, else it is a StrongRef which is a locally defined class.

I tried to add a unit test which reproduced the issue by garbage collecting values in the cache so they would be Refs to undefined, but I didn't have any luck. I followed a similar strategy to this test by using a FinalizationRegistry. I also tried a test which unmounted a React component containing the original state. Please let me know if you have any ideas for this unit test :)

(aside) How we discovered the issue:

netlify[bot] commented 1 month ago

Deploy Preview for reselect-docs canceled.

Name Link
Latest commit cea19b94cd2bf33c2350de821c74f38c8980cf00
Latest deploy log https://app.netlify.com/sites/reselect-docs/deploys/669e7cb1ff5f5700083e6007
codesandbox-ci[bot] commented 1 month ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.