Fixed single arg flaw with string types. #65
Conversation
|
This probably failed your build because I added a unit test that will fail with your legacy code. |
|
This pull request includes the previous pull request to fix the string key flaw. It also more than doubles the speed of fast-memoize in the single argument case. My guess is you will get a jump in multi-arg as well, but I don't thoroughly understand your benchmark suites, so I will leave it up to you to check. |
|
This change broke my use of |
| if (!cache.has(cacheKey)) { | ||
| var computedValue = fn.call(this, arg) | ||
| var computedValue = cache.get(cacheKey); | ||
| if (typeof computedValue==="undefined") { |
There was a problem hiding this comment.
This seems to be sacrificing API correctness for speed.
There was a problem hiding this comment.
@megawac I would say most of my approach does sacrifice API correctness for speed, but the stated goal of the library is to provide the "fastest possible memoization library in JavaScript that supports N arguments." I was just working in concert with its stated goal. Also, the failure to treat string keys correctly was in my opinion a fundamental semantic/functional flaw. Take a look at: https://codeburst.io/a-key-to-keys-when-javascript-keys-dont-match-ab44c81adc87.
Your current code is subject to this flaw: https://codeburst.io/a-key-to-keys-when-javascript-keys-dont-match-ab44c81adc87
The pull request fixes the flaw with marginal impact on performance.