Respect function argument count #18
Conversation
Codecov Report
@@ Coverage Diff @@
## master #18 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 1 1
Lines 16 19 +3
=====================================
+ Hits 16 19 +3 |
src/index.js
Outdated
| @@ -33,6 +33,12 @@ export default function <ResultFn: (...Array<any>) => mixed>(resultFn: ResultFn, | |||
| return lastResult; | |||
| }; | |||
|
|
|||
| Object.defineProperty(result, 'length', { | |||
There was a problem hiding this comment.
could you just do result.length = resultFn.length?
There was a problem hiding this comment.
No, function.length is readonly property, reflecting the numbers of arguments. The only way to override it - define a descriptor.
Actually, the best way here is to pass through all the descriptors from the original function, including name, and any static prop, that may exist, but I decide to keep your code compact.
| it('should maintain function arguments count', () => { | ||
| expect(memoizeOne(a => a).length).to.equal(1); | ||
| expect(memoizeOne((a, b) => a + b).length).to.equal(2); | ||
| expect(memoizeOne((...rest) => rest).length).to.equal(0); |
There was a problem hiding this comment.
should this equal 0?
and should the next one equal 1?
There was a problem hiding this comment.
No, the final form of this function operates on arguments, not leaving anything in the function signature.
That's the problem.
|
function1 -> memoized_function1 Follow up -> may be .toString should return the original toString, as long lots of libraries detect arrow or bound functions using string representation of a function. |
src/index.js
Outdated
| @@ -3,6 +3,13 @@ type EqualityFn = (a: mixed, b: mixed) => boolean; | |||
|
|
|||
| const simpleIsEqual: EqualityFn = (a: mixed, b: mixed): boolean => a === b; | |||
|
|
|||
| const defineProperty = (target, property, value) => | |||
There was a problem hiding this comment.
can you please add some flow types to this function?
(target: Object, property: string, value: mixed): void
| @@ -33,6 +40,10 @@ export default function <ResultFn: (...Array<any>) => mixed>(resultFn: ResultFn, | |||
| return lastResult; | |||
| }; | |||
|
|
|||
| defineProperty(result, 'length', resultFn.length); | |||
|
|
|||
| defineProperty(result, 'name', `memoized_${resultFn.name || 'one'}`); | |||
There was a problem hiding this comment.
I am a little worried that this might slow things done a bit. I am not sure if the effort is worth the sugar. Would you mind taking a look at the impact?
There was a problem hiding this comment.
It is possible to make this getter. Then it shall not have any impact.
Anyway - it is easy to test.
There was a problem hiding this comment.
Perhaps we could make them both getters! As you said - worth testing!
Thanks @theKashey!
| @@ -33,6 +40,10 @@ export default function <ResultFn: (...Array<any>) => mixed>(resultFn: ResultFn, | |||
| return lastResult; | |||
| }; | |||
|
|
|||
| defineProperty(result, 'length', resultFn.length); | |||
There was a problem hiding this comment.
but we should totally ship the length property!
|
Any update on this one @theKashey ? |
|
Yep, sorry - forgot to update. I've tried to stress test 2 versions of memoize-one, and the delta was inside "delta", ie 1-2%. |
|
Thanks @theKashey !!! |
A lot of libraries relay on arguments count - react-redux will deoptimize mapStateToProps if argument count is not 1, fast-memoize changes it's logic(and stops working as long arg count is 0), and so on.
The idea of this PR is simple - just pass down the original function length. As long it is readonly - do it via descriptor.