use-inline-memo icon indicating copy to clipboard operation
use-inline-memo copied to clipboard

deps default to []

Open neves opened this issue 6 years ago • 4 comments

Awesome package. Seriously, it should be part of React core! Suggestion: Instead of: memo.textChange(event => setValue(event.target.value), []) accepts this: memo.textChange(event => setValue(event.target.value))

Question: How can you measure that the overhead in memo is faster then a component rerender that justify the use of it?

Great work!

neves avatar Aug 31 '19 07:08 neves

Thanks, @neves!

deps default to []

Yeah, let's openly discuss that. I was thinking about a default in the beginning, but then figured that it might be better to explicitly fail than just silently falling back to re-using the first value forever.

So I think it comes down to "Optimizing for the probably most prominent use case" vs. "Explicit is better than implicit".

andywer avatar Aug 31 '19 09:08 andywer

How can you measure that the overhead in memo is faster then a component rerender that justify the use of it?

Good question. You can benchmark the performance of a component using a HOC like that one:

https://github.com/andywer/use-inline-memo/blob/master/test/integration.test.tsx#L9-L17

The question remains how to switch the memoization on and off. We could just let you set an environment variable to disable all memoization. You benchmark all the components you want to and compare with the env var set and not set.

It would be preferable to enable/disable memoization on a component-level, but I don't see an easy way to accomplish that.

The only thing that could be done easily is to provide a helper useInlineMemo.disable() which comes with the same API as useInlineMemo() but returns an object where all functions just blindly pass-through the input value.

andywer avatar Aug 31 '19 09:08 andywer

So I think it comes down to "Optimizing for the probably most prominent use case" vs. "Explicit is better than implicit".

In that case, we are back in square one, because the correct explicit way, should be: memo.textChange(event => setValue(event.target.value), [setValue]) Not in the case of setValue from useState, but for a setValue from a custom hook, where the author didnt used useCallback to memoize the setter.

Maybe this if in the code (that split it in two different factories) is telling us something: https://github.com/andywer/use-inline-memo/blob/master/src/index.ts#L114

neves avatar Sep 03 '19 09:09 neves

memo.textChange(event => setValue(event.target.value), [setValue])

Yes, that's what I wanted to discuss: Explicit version (as it is right now) vs. implicit, but maybe a little more convenient 😉

Maybe this if in the code (...) is telling us something

You are talking in riddles... What has the ES5 implementation vs ES6 proxy implementation switch to do with the memoization function API discussion?

andywer avatar Sep 04 '19 07:09 andywer