purescript-react / purescript-react-basic-hooks

An implementation of React hooks on top of purescript-react-basic
https://pursuit.purescript.org/packages/purescript-react-basic-hooks/
Apache License 2.0
198 stars 31 forks source link

Use Callback #54

Open codingedgar opened 2 years ago

codingedgar commented 2 years ago

Hi! Thanks a lot for this amazing package ✨

I cannot find an example of React.useCallback I see it was deprecated on what seems to be v5.x what is the correct way to "useCallback" in a hook?

Sorry if it is a silly question I'm learning purescript

pete-murphy commented 2 years ago

Seems like you could just use useMemo?

  fn' <- Hooks.useMemo deps \_ -> fn

would be the same as a hypothetical

  fn' <- Hooks.useCallback deps fn

right?

codingedgar commented 2 years ago

Wow idk why the issue description was cut.

What i meant to say was: i see useCallback existed until v5 and now it’s gone, why? Is there a better way to do useCallback in PureScript?

codingedgar commented 2 years ago

Seems like you could just use useMemo?


  fn' <- Hooks.useMemo deps \_ -> fn

would be the same as a hypothetical


  fn' <- Hooks.useCallback deps fn

right?

It seems about right, but the same applies in regular js, yet useCallback is exposed, why remove it? Coming from Reactjs is a bit weird not to have the same API.

I guess it's because it is "unnecessary" or something similar in PureScript?

megamaddu commented 2 years ago

Yeah, it's a combination of me naming some functions badly in previous versions plus it being redundant. Leaving it in would have caused confusing errors for those upgrading. I'm not opposed to adding it now, but also.. it's literally useMemo with the wrapper fn applied for you πŸ˜…

codingedgar commented 2 years ago

I was wondering more if the whole pattern was unnecessary or something, meaning declaring functions inside the body of a component does not cause React to re-render sub-components diffing a new function all the time, etc, but if it is the same case then having useCallback seems nice, it's something pretty well understood coming from reactjs to react-basic-hooks.

Yeah, it's a combination of me naming some functions badly in previous versions plus it being redundant. Leaving it in would have caused confusing errors for those upgrading. I'm not opposed to adding it now, but also.. it's literally useMemo with the wrapper fn applied for you πŸ˜…

That's fine, considering PS commonality of having a bunch of ergonomic signatures (like fn, fn_, fn') I don't see why it would be bad to have useCallback as a shorthand, but I'm so noob in PS that maybe useCallback is not as widely used/necessary in PS as it is in JS?

But it's fine, I was mostly wondering about the former, I would "vote" for having it, but if it is trouble or confusing in PS, that's fine too, no need to make a change just for that.

megamaddu commented 2 years ago

That's true, I don't mind adding it.

For a little more background, I think PS devs are less likely to be thinking about reference equality at all, especially for functions, and just eating that performance cost until it actually becomes a problem. On the flip side, it's become fairly expected in JS-land and the more you see it the more you keep using it. In most cases it's a premature optimization. You can reach for these tools in PS if you need them (memo, UnsafeReference, useMemo, and [if added] useCallback), but it isn't as common.

andys8 commented 2 years ago

It would be great if there would be useCallback: It is also documented in the react ecosystem and people might look for it. On the other hand it is helpful for people who don't know and are wondering why such a function exists and what problem it solves.

What option would you prefer @megamaddu ?

Adding useCallback implemented in terms of useMemo.

useCallback :: forall deps a. Eq deps => deps -> a -> Hook (UseMemo deps a) a
useCallback deps fn = useMemo deps (const fn)

or actually calling useCallback from the react library and adding a UseCallback hook.

Even though react docs say it's equivalent to, react team could decide to change internals of both functions or remove one of them in a breaking change in the future. And I personally tend to just exposing these functions from the react library and would go with the latter option (calling useCallback from the react library).

image