Use a WeakMap to store memoized values #298
Conversation
Codecov Report
@@ Coverage Diff @@
## master #298 +/- ##
==========================================
- Coverage 91.91% 91.87% -0.04%
==========================================
Files 18 18
Lines 408 406 -2
Branches 76 75 -1
==========================================
- Hits 375 373 -2
Misses 19 19
Partials 14 14
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
src/memoize.js
Outdated
|
|
||
| /** | ||
| * There is only ever a single ctx object in the browser. | ||
| * Therefore we can use a simple memoization function. |
There was a problem hiding this comment.
I'm not totally sure if this assumption is valid. This is certainly the case in real applications, but I wonder if this may not be true with simulation tests that run in the browser (although I don't think this is a thing).
But if this assumption is not valid, we should add a test case for this.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/memoize.js
Outdated
| return browserMemoize(fn); | ||
| } | ||
|
|
||
| const wm = new WeakMap(); |
There was a problem hiding this comment.
We could put this logic be in an else block, and/or do feature detection in browser
There was a problem hiding this comment.
Actually, we ship core-js polyfills when necessary, so perhaps we can just assume WeakMap exists.
There was a problem hiding this comment.
Polyfill doesn't GC though, does it?
There was a problem hiding this comment.
I'm not sure but it the core-js polyfill seems to use this weak collection implementation (which I believe should allow for GC): https://github.com/zloirock/core-js/blob/master/packages/core-js/internals/collection-weak.js
Using a WeakMap to store memoized values obviates the need for having an actual property on context.