Closed markerikson closed 9 months 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.
Latest deployment of this branch, based on commit 8a6eb42d1190b5d8c3f94f63642d473ba451137e:
Sandbox | Source |
---|---|
Vanilla Typescript | Configuration |
Cleaned this up some:
weakMapMemoize
["object", "function"].includes()
call to be if
statements and inside the if (resultEqualityCheck
clauseOutputSelector
generic rearranging
This PR:
jsdom
InputSelectors
generic arg ofOutputSelector
to be last, because it seemed like it was more ergonomic that way. But then I used it and I'm not sure about that.@internal
prefix from a couple imports because Vitest seemed to be having trouble with this and it doesn't seem to serve any useful purpose'reselect'
in the new.tsx
test file I addedresultsCount()
getter todefaultMemoize
. We currently track recomputations at the selector level, but not how many new results the memoized function returned (because it may have reused existing results ifresultsEqualityCheck
found a match)weakMapMemoize
to try to addresultsEqualityCheck
as an optiontbh I'm really not happy with where this stands. It seems to work, mostly. But we're inherently limited by the way we can't check vs existing cache entries, since we don't have the args lying around. With
defaultMemoize
we have all the existing cache entries in an array.I could imagine writing some code that keeps existing results in a
WeakRef
of some kind (Lenz's suggestion), or traversing the cache nodes tree. But there could be N existing cache entries to compare against, and that's also the behavior that causesdefaultMemoize
with amaxSize > 1
to be kinda slow.So I ended up with a single
lastResult
value for the whole memoized function, and we compare against that. My working assumption is that either you need this to be a cache size of 1 and you want consistent reused results if possible, or you want an infinite cache size and aren't going to be trying to reuse anything.I added a test case with a React todo list. It currently runs a few combos of default vs weakMap, standard vs with
resultsEqualityCheck
, and logs the resulting renders and recomputations to the console. I specifically forcedselectTodoById
to return[todo]
as an array to force a new reference. This wouldn't normally be the case for a straight lookup.Here's my latest results:
I think this says that with
weakMapResultEquality
, the list only re-rendered when we added an item (expected) and not when we toggled completed (good).Concerns
Right now I have no tests. I am also not 100% sure the actual logic is correct.
I'm really feeling uncomfortable about where things stand atm.
I'd still seriously like to make the switch to
weakMapMemoize
as default in Reselect 5.0. But I think it needs to have some form ofresultEqualityCheck
, both to keep API compat with (the relatively few) folks who are doingcreateSelector(a, b, {memoizeOptions: {resultEqualityCheck}})
in the wild, and also because it's a use case thatdefaultMemoize
made handleable in 4.1 and I don't want to regress on that aspect.but what I've got over here is very hacked up and doesn't particularly feel ready.
@aryaemami59 , can you take a look at this and give some thoughts?