andywer / use-inline-memo

⚛️ React hook for memoizing values inline anywhere in a component
164 stars 3 forks source link

deps default to [] #4

Open neves opened 5 years ago

neves commented 5 years ago

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!

andywer commented 5 years ago

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 commented 5 years ago

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.

neves commented 5 years ago

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

andywer commented 5 years ago

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?