useCallback usage

[AjaxSolutions]

Why the get, set, remove and remove functions are not wrapped in useCallback?

https://github.com/streamich/react-use/blob/master/src/useMap.ts

The same question applies to functions returned by useList.

[streamich]

What is your use case?

[Granipouss]

Hi,

I tried to use useMap to hold values of a form and the fact that the methods were not memoized basically made the form unusable due to constant rerenders.

IMO, it would be a good idea to propose the most stable (i.e. memoized) return values possible. Similar to the way useState and useReducer setter and dispatcher are memoized.

Cheers,

[Belco90]

I agree with @Granipouss: ideally all these returned callbacks should be memoized to always return same instance if same args provided. We have fixed few of them, in previous PRs like this one #570

@AjaxSolutions could you create a PR for fixing those callbacks on useMap hooks? Thanks.

[xobotyi]

Looked inside the hook and got trrified🙈
Starting from non-memoized methods, ending with object recreation each insert-delete it will cause tramendous performance breakdown on more-or-less big amount of entities.

IMO it should use ref to a storage value, this way it can have the maximum performance and the only callbacks instance with no need to update them ever.
@Granipouss @Belco90 @streamich what do you think? If everyone agree i'd like to take it. Remastering the code would be pretty simple for this one.

[Belco90]

Callbacks returned should be memoized, but I'm not so sure the actual object should be memoized too. You need to return a new object when updated, otherwise the component using this hook won't know when the object has been updated. Think about React.setState using an object: everytime you set something on it, it returns a new object so you can react to changes in your component. I don't know if I misunderstood what you meant.

[xobotyi]

@Belco90 the component re-rendering can be done with a single useUpdate hook whose call can be done in handles (forgot to mention in the comment %)).

[Belco90]

Sure, but that's re-implementing the natural behavior of React.useState by 1) manually storing the object on ref and 2) manually triggering an update, when useState already handles that (at the end useMap is a just a shortcut for useState with an object and manipulation callbacks.

I would go for leaving the setState and just memoize the callbacks. As you mentioned, should be easy to solve, all current tests should pass and you can add a new one to check callbacks returned are always same instance.

[xobotyi]

@Belco90 the ref is needed to solve performance issue that, i believe, will appear already on 500-1000 entities inside the state.

[Granipouss]

As far as I know, multiple useCallback(..., []) on actions, a singular useMemo(..., []) on the action object or a useRef are very similar and would all solve most of the issues.

I believe the actions are meant to be used with destructuring (could be wrong tho), so I see no need to memoized the returned object. It would be safer tho.

[Belco90]

@xobotyi I don't think that would raise a performance issue using useState as that would mean react itself would have a performance problem. So I think the same: implement it with react useState to avoid reimplementing anything and just memoize the callbacks.

[Belco90]

I just found this and seems interesting for identifying which React hook suits better for each case :) https://twitter.com/tylermcginnis/status/1169667360795459584