Initial hooks implementation #159

ethul · 2018-11-25T14:56:23Z

Proposal for supporting React Hooks.

This code is still in the early stages and requires some ideas to be worked out further, but perhaps it is a useful start for a discussion. Any comments, questions, or suggestions are most welcome.

// @natefaubion

natefaubion · 2018-12-01T19:06:32Z

src/React/Hook.purs

+useState
+  :: forall a
+   . a
+  -> Hook (Tuple a (SetState a))


I'd be interested to see if the state handling internally is fully parametric or if it still has all the sugar around copying/merging properties that component setState uses.

I think one reason why this can't be parametric is that the setState state function is overloaded to either take the state as is or a function, which means you can't have a anything represented by a function as state.

I believe this is the relevant section handling the state:
https://github.com/facebook/react/blob/52bea95cfce4a75bc7c1a09455abaeef50fa0700/packages/react-reconciler/src/ReactFiberHooks.js#L333-L472

The only thing I see is that the code checks if the state is a function:
https://github.com/facebook/react/blob/52bea95cfce4a75bc7c1a09455abaeef50fa0700/packages/react-reconciler/src/ReactFiberHooks.js#L455-L457

As you mention it seems the overloading is problematic. I am open to ideas or suggests for the representation on this.

Unfortunately there's no safe way to describe the API. One option would be to enumerate JS primitives. This is conservative.

foreign import data StateValue :: Type -> Type intValue :: Int -> StateValue Int intValue = unsafeCoerce stringValue :: String -> StateValue String stringValue = unsafeCoerce recordValue :: forall r. { | r } -> StateValue { | r } recordValue = unsafeCoerce useState :: forall a. StateValue a -> Hook (Tuple a (SetState a))

This could potentially be overloaded with an instance chain that closes it off with a custom type error. You could then expose an unsafeUseState function which documents the caveat that you can't use anything which has a function as it's representation.

I like the idea of enumerating the primitives, and maybe this is the best option. However, what you think about testing for a function and wrapping the passed in state in a function if detected. Maybe this is a bit too unsafe, but I think it might work.

natefaubion · 2018-12-01T19:10:18Z

src/React/Hook.purs

+useEffect
+  :: forall a
+   . Effect (Effect a)
+  -> Maybe (Array EffectInput)


Is there a reason to not go ahead and lose the Maybe. There are two "empty" cases with Just []. If a user always has to pass in something they might as well pass in [].

I believe that React behaves differently if you pass an empty array vs. not passing an array at all.

https://reactjs.org/docs/hooks-reference.html#conditionally-firing-an-effect
Not passing an array means that the effect is fired after every render. Passing an empty array means that the effect is fired only on mount and unmount.

natefaubion · 2018-12-01T19:17:59Z

src/React/Hook.purs

+callbackInput :: forall a. a -> CallbackInput
+callbackInput = unsafeCoerce
+
+foreign import data CallbackInput :: Type


Since this is the same as EffectInput, maybe there should be a shared MemoValue or something that they both use instead. The only reason this exists is to erase the type, so even Foreign would be appropriate, but I don't think it's necessary to have new types for each one.

I started with a shared type for these. I would be up for making one type. Probably cleaner that way. Good call.

natefaubion · 2018-12-01T19:20:16Z

src/React/Hook.purs

+   . (Unit -> a -> b)
+  -> Maybe (Array MemoInput)
+  -> Hook (a -> b)
+useMemo k = runFn2 useMemo_ k <<< Nullable.toNullable


I don't think useMemo necessarily computes a function. It is supposed to memoize an arbitrary value (useCallback is just a shortcut for memoizing a function).

Gotcha. I will review this. I haven't played with it too much yet. Thanks.

natefaubion · 2018-12-01T19:24:56Z

src/React/Hook.purs

+   . Ref a
+  -> (Unit -> { | r })
+  -> Maybe (Array ImperativeMethodsInput)
+  -> Hook Unit


I'm not sure about this type. useImperativeMethods depends on forwardRef, which will pass in a second argument to your component. We don't expose this anywhere, so I don't know if Ref a is the correct type. I think it would actually be very hard to make use of this in even a remotely type-safe way.

Agreed that this may not be the best representation. I still need to work this one out a bit more. I am open to suggestions on this.

natefaubion · 2018-12-01T19:30:15Z

src/React/Hook.purs

+         (Hook Unit)
+
+useContext :: forall a. Context a -> Hook a
+useContext = runFn1 useContext_


Right now, this isn't usable since Context is a foreign type here.

In hindsight, I should have made Context a foreign type to begin with instead of wrapping it.

Maybe it is worth making Context a foreign type? It's a breaking change, but maybe it is an idea.

I'm OK with the breaking change. It's very minor.

Sounds good. I've changed Context over to a foreign type.

Also, in order to render the consumer, I found I needed a createRenderPropsElement function. Was there another way to render the consumer?

On a side note, it seems refs have also been updated in React 16.3. I've incorporated those changes as well since it fits in with the hooks API.

I've just used createLeafElement, and supplied the children prop explicitly in those cases.

Ah, okay. Having createRenderPropsElement might be useful. What do you think? I am not sure about the name though.

natefaubion · 2018-12-01T19:34:24Z

src/React.purs

+  :: forall props
+   . ({ | props } -> Hook ReactElement)
+  -> { | props }
+  -> ReactElement


createHookElement like createLeafElement?

Since this all uses createElement under the hood, this isn't parametric due to key and ref props.

Sounds good for createHookElement. Noted on key and ref.

ethul · 2018-12-01T22:34:41Z

@natefaubion Thanks for taking a look at all of this!

ethul · 2018-12-02T14:48:49Z

Example usage of hooks: https://github.com/ethul/purescript-react-example/tree/hooks

natefaubion · 2018-12-08T23:30:28Z

src/React.purs

@@ -332,7 +331,7 @@ class ReactPropFields (required :: # Type) (given :: # Type)

 type ReservedReactPropFields r =
  ( key :: String
-  , ref :: SyntheticEventHandler (Nullable ReactRef)
+  , ref :: Ref DOMRef


I don't think this is correct. A class component isn't going to have a DOMRef since it will be pointing to the instance. As is, the only thing that would make sense to me is Foreign. Since we are redoing refs anyway, maybe there's a better way to handle this side of the interface in a typed way... I'm not sure though.

You're right. It could be the instance. I wouldn't mind Foreign, but what if we made an opaque type ForwardRef instead? I am not sure if there is a better way to hand this, but I am open to ideas.

natefaubion · 2019-02-06T19:43:41Z

It's definitely worth looking at the react-basic implementation which uses indexed-monads and qualified do to make it ergonomic and type safe.

ethul · 2019-02-17T20:41:15Z

Thanks for pointing out the react-basic hooks implementation. It is a nice representation. A part of me wonders if it would make sense to have a more general hooks implementation that can be shared among purescript react libraries.

On another note, and as a point for discussion, I suppose I am a bit on the fence about tracking hooks via the index-monad, which seems along the lines of how Eff tracked effects (unless I am not understanding this correctly). I can see the how this could be handy, but I may need to mull over the pros and cons a bit more. But I am open to ideas on this.

JordanMartinez · 2021-09-23T14:43:47Z

Should this PR be closed? AFAIK, most people use react-basic now, which also has a hooks library.

ethul added 9 commits November 25, 2018 09:47

Initial hooks implementation

90e61f0

Remove Action type

0e5d81b

Fix remove Action

1088218

Flip createElementHooks arguments

53d8011

Remove re-export

102d044

Use Hook type

a60a253

Export createElementHooks

e29447e

Rename Hooks to Hook

e39a96c

Use Effect

140c3bc

natefaubion reviewed Dec 1, 2018

View reviewed changes

ethul added 5 commits December 1, 2018 18:21

Updates based on feedback from @natefaubion

4e456da

Fix createHookLeafElement

2c1a2cc

Update unsafeCreateHookLeafElement

088d175

Update useMemo

7b5ec57

Fix dispatch

e8b5738

ethul added 3 commits December 8, 2018 15:04

Refactor Context

edcd569

Add createRenderPropsElement context consumer creation

ba17b83

Refactor Refs

ebe5f47

natefaubion reviewed Dec 8, 2018

View reviewed changes

ethul mentioned this pull request Apr 24, 2019

Add support for createRef #167

Closed

thomashoneyman changed the base branch from master to main October 6, 2020 02:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial hooks implementation #159

Initial hooks implementation #159

ethul commented Nov 25, 2018

natefaubion Dec 1, 2018

natefaubion Dec 1, 2018 •

edited

Loading

ethul Dec 1, 2018

natefaubion Dec 2, 2018

ethul Dec 9, 2018

natefaubion Dec 1, 2018

ethul Dec 1, 2018

natefaubion Dec 2, 2018

natefaubion Dec 1, 2018

ethul Dec 1, 2018

natefaubion Dec 1, 2018

ethul Dec 1, 2018

natefaubion Dec 1, 2018

ethul Dec 1, 2018

natefaubion Dec 1, 2018

natefaubion Dec 1, 2018

ethul Dec 1, 2018

natefaubion Dec 2, 2018

ethul Dec 8, 2018

natefaubion Dec 8, 2018

ethul Dec 9, 2018

natefaubion Dec 1, 2018

natefaubion Dec 1, 2018

ethul Dec 1, 2018

ethul commented Dec 1, 2018

ethul commented Dec 2, 2018

natefaubion Dec 8, 2018

ethul Dec 9, 2018

natefaubion commented Feb 6, 2019

ethul commented Feb 17, 2019

JordanMartinez commented Sep 23, 2021

Initial hooks implementation #159

Are you sure you want to change the base?

Initial hooks implementation #159

Conversation

ethul commented Nov 25, 2018

Choose a reason for hiding this comment

natefaubion Dec 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethul commented Dec 1, 2018

ethul commented Dec 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natefaubion commented Feb 6, 2019

ethul commented Feb 17, 2019

JordanMartinez commented Sep 23, 2021

natefaubion Dec 1, 2018 •

edited

Loading