Replace old ReasonReact-like API with Hooks #27

wokalski · 2019-01-12T00:50:17Z

This PR introduces basic support for a React-Hook like API. The only supported hooks are useState and useReducer. Implementing useEffect will probably require some architectural overhaul to the current naive Slots approach. Also, there's a lot
of mutation everywhere now. It'd be great to minimise this in the next version.

Temporary component API

If you look at the tests the component looks like this:

let component = component(debugName);
let make = (~prop, children) => component(slots => { ... });
let createElement = ...

This API is temporary because it doesn't require a PPX for JSX. The planned API is:

let componentName = component((slots, ~prop, ~prop2, ()) => { ... })

core/lib/Slots.re

jchavarri

This looks amazing @wokalski !! It's really great to see how hooks simplify many parts of the code. I was also surprised by how the implementation of hooks in Hooks.re takes just a few lines 😮

I had just a few questions, that maybe others reviewing the PR could also share.

core/lib/Hooks.re

core/lib/RemoteAction.rei

core/lib/ReactCore_Internal.re

core/lib/Slots.re

tester-macos/bin/app.re

wokalski · 2019-01-12T18:36:56Z

@jchavarri do you have any ideas how to implement a PPX rewriter for this hook API?

jchavarri · 2019-01-12T18:50:41Z

@wokalski I had a wip branch called hooks-ppx in reason-reactify: jchavarri/reason-reactify@master...jchavarri:hooks-ppx

Note this ppx was highly experimental, and it was applied to the continuation-passing style API we explored at some point. But it was working for that case (at least the happy path), and it might help as starting point for the slots ppx.

Relevant files: ppx/dune, ppx/hook.re and the package.json for the two deps (ocaml-migrate-parsetree and ppx-tools).

One thing I was also planning to add are some tests for it. I was taking a look at a section called "Testing the rewriter" in http://rgrinberg.com/posts/extension-points-3-years-later/#testing-the-rewriter.

wokalski · 2019-01-12T19:20:45Z

@jchavarri i see. My main conceptual problem with the ppx is how do we pass the initial slots value

jchavarri · 2019-01-13T22:21:25Z

@wokalski One would need two ppxs: one for the component and one for the hook.

For example:

let%comp counter = (_children) => {
  let%hook (counter, setState) = Hooks.useState(0);
  <Button onClick={() => setState(counter + 1)} />;
};

or with a module ppx, which is maybe more "legit" about what is happening behind the scenes, but still can alleviate all the work of manually calling component, creating make using it, and adding createElement:

module%comp Counter = {
  let render = _children => {
    let%hook (counter, setState) = Hooks.useState(0);
    <Button onClick={() => setState(counter + 1)} />;
  }
};

both would be transformed into:

module Counter = {
  let component = component("Counter");
  let make = _children =>
    component(_slots => {
      let (counter, setState, _slots) = Hooks.useState(0, _slots);
      <Button onClick={() => setState(counter + 1)} />;
    });
  let createElement = (~children, ()) => element(make(children));
};

The %comp ppx could hide more or less surface from the underlying API.

wokalski · 2019-01-13T23:31:13Z

@jchavarri great! I think we're on the same page, I've also been thinking about such PPX! I'm not sure what should be the "custom" hook ppx. I.e. how to name it. The bonus point of a ppx is that we can use an invalid ocaml identifier as the variable name - it's very unlikely for it to shadow anything in such case!

jchavarri · 2019-01-14T08:37:08Z

I'm not sure what should be the "custom" hook ppx

I guess you mean "custom" component ppx? Maybe we could use let%render or module%render, as "render" is kind of a universal React term for that? The downsides I see from the let% ppx (as opposed to the module% one) are:

the lowercase/uppercase issue: let statements only allow lowercase identifiers, but uppercase components are the common idiomatic approach in ReactJS. That's why I feel more inclined to use the module% ppx (although it's more verbose 🤔 )
module% is more transparent / honest about what's happening behind the scenes

The bonus point of a ppx is that we can use an invalid ocaml identifier as the variable name

For the slots identifier, right? That's a great point! Using an invalid identifier removes all chances of collisions 💯

wokalski · 2019-01-14T09:04:28Z

@jchavarri I meant the custom hook, ie custom hook function. I realise it's "just a function" but if we use a special identifier for let%hook there has to be a ppx for the definition of such function to provide the hook arg with the same identifier!

When it comes to the component ppx I guess we have to see. I'm more inclined towards lower-case components because they are less verbose and maybe we could leave the upper case PPX for something else!

jchavarri · 2019-01-14T10:03:41Z

there has to be a ppx for the definition of such function to provide the hook arg with the same identifier

What do you mean? Wouldn't the author of the custom hook decide whatever name for slots they prefer? The only constraint that I see is that the slots param needs to be the last one.

wokalski · 2019-01-14T10:42:14Z

@jchavarri if let%component c = (~prop, ()) => { ... } desugars to let c = component((@@hooks, ~prop, ()) => { ... } then let%hook x = useState(prop) has to desugar to let (x, @@hooks) = useState(prop, @@hooks). I.e. the unique "invalid" identifier that let%hook picks up has to be consistent. So, when you're making a "custom" hook, you have to somehow supply the hook with this unique variable name.

jchavarri · 2019-01-14T11:30:34Z

the unique "invalid" identifier that let%hook picks up has to be consistent.

I agree. 👍

Maybe I was confused because I wasn't thinking custom hooks authors would use the let%hook ppx, I see that ppx useful for consumers of hooks, not creators.

i.e. I was thinking something like this:

/* Some custom hook, doesn't use any ppx */
let useCustomHook = (prop, slots) => { let foo = useState(prop, slots); ... }

let%component c = (~prop, ()) => { let%hook x = useCustomHook(prop) }

core/lib/Slots.rei

core/lib/Slots.re

wokalski · 2019-01-16T13:10:03Z

The data structure used for accumulating enqueued effects is extremely inefficient and there are almost no tests for the effect hook. That said I would like to address those issues in a separate PR. I would love to, however, get some feedback on the implementation of hooks themselves and what you think about the API. I think the most relevant modules are: Slots, Hooks, and ReactCore.rei. Please keep in mind that we have not implemented a PPX for the JSX yet but it's high on our priority list.

bryphe · 2019-01-17T00:01:36Z

core/test/Assert.re


 let executeSideEffects = ({renderedElement} as testState) => {
  RenderedElement.executeHostViewUpdates(renderedElement) |> ignore;
+  List.iter(f => f(), renderedElement.enqueuedEffects);


It looks like enqueuedEffects isn't available on the renderedElement via ReactCore.

Is it expected that these effects would be ran as part of executeHostViewUpdates? Or will there be a separate API, like executeEffects?

There has to be a separate API. The reason is that we normally want to call executeHostViewUpdates on the main thread but the effects should be executed on a background one.

bryphe · 2019-01-17T02:04:06Z

The data structure used for accumulating enqueued effects is extremely inefficient and there are almost no tests for the effect hook. That said I would like to address those issues in a separate PR.

Makes sense. The 'Effect' API looks good to me, aside from one comment I left - I'm wondering if there will be a separate API for executing the effects, or if its expected to occur as part of executeHostViewUpdates?

Aside from that - seems like the API surface will be the same, and its just about fixing bugs / streamlining the syntax with PPX / etc. 👍

Thanks for all your hard work on this @wokalski !

jchavarri

This looks awesome @wokalski !! 👏 👏 👏

jchavarri · 2019-01-17T08:03:12Z

core/lib/Hooks.re

@@ -0,0 +1,266 @@
+type hook('a) = ..;


Just curious, what's the advantage of the "open variant" approach? as opposed to define hook('a) after the modules?

I don't know... yet. I thought it might be an interesting idea to implement hooks as an open variant to allow users creating custom types of hooks in the userspace. Like what if we exposed enough internals (but not too much!) to implement Hooks.suspense in the user space? I think it could be very interesting.

jchavarri · 2019-01-17T08:14:43Z

core/lib/Hooks.re

+    stateContainer := {currentValue, updates: [nextUpdate, ...updates]};
+  };
+
+  let hook = (~initialState, reducer, hooks: hooks(_)) => {


nit: initialState is not a named arg in Ref and State.

That's on purpose. Ref and State are single argument + hook so it's pretty self explanatory and for the reducer you know you have to put the reducer in but you might forget the order for instance so we add this explicit ~initialState argument.

jchavarri · 2019-01-17T08:18:23Z

core/lib/Hooks.re

+  onSlotsDidChange,
+};
+
+module State = {


As a fun idea, in reason-reactify we used to implement useState as a special case of useReducer which allowed to reduce some of the boilerplate. Not sure if it'd be interesting in this case :)

https://github.com/revery-ui/reason-reactify/blob/38e59a7726146bdb63e4b53bb3068bab6132aa95/lib/Reactify.re#L565-L577

they were implemented differently on purpose. My thought process is that the operation in the reducer might take relatively long to compute and actions are very often fired during interactions. Enqueuing an update takes constant time. Also, with this approach it'll be relatively easy to implement UpdateWithSideEffects. That said I'm not sure about this myself 😕

jchavarri · 2019-01-17T08:20:28Z

core/lib/Hooks.re

+  let enqueueUpdate = (nextUpdate, stateContainer) => {
+    let {currentValue, updates} = stateContainer^;
+
+    stateContainer := {currentValue, updates: [nextUpdate, ...updates]};


Why does Reducer follow a different strategy than State? Would it be possible to instead of a list of updates, call the reducer and store the value in nextValue as the state hook does?

jchavarri · 2019-01-17T08:26:12Z

core/lib/Hooks.rei

+    | Update;
+  type always;
+  type onMount;
+  type condition('a) =


Trying to wrap my head around this... if this is already a GADT, does the 'a need to be exposed? Would something like this be possible?

type condition = | Always: condition | OnMount: condition | If(('a, 'a) => bool, 'a): condition;

Or alternatively, make it a regular variant / ADT.

I guess I'm not understanding why this is a GADT 😄

Good question. The reason is that I want the constructor to carry more type information so that you cannot do this:

Hooks.effect(someProp ? Always : OnMount, () => None)

Always and OnMount have different types so the semantics here would be very confusing!

also, for the if condition we have to know what type the compared value is!

jchavarri · 2019-01-17T08:32:32Z

core/lib/Slots.re

+
+module type S = {
+  type elem('a);
+  type opaqueElement =


I got slightly confused about element. It's not the reconciler "element", right? This would be more the "content" or the "stored data type" the slots will contain? 🤔

great suggestion!

Fixed!
cc24a0a

This commit should also the reduce of allocations.

wokalski added the core Core label Jan 12, 2019

cristianoc reviewed Jan 12, 2019

View reviewed changes

core/lib/Slots.re Outdated Show resolved Hide resolved

jchavarri reviewed Jan 12, 2019

View reviewed changes

core/lib/Hooks.re Outdated Show resolved Hide resolved

core/lib/RemoteAction.rei Show resolved Hide resolved

core/lib/ReactCore_Internal.re Show resolved Hide resolved

core/lib/Slots.re Outdated Show resolved Hide resolved

tester-macos/bin/app.re Outdated Show resolved Hide resolved

cristianoc reviewed Jan 14, 2019

View reviewed changes

core/lib/Slots.rei Outdated Show resolved Hide resolved

cristianoc reviewed Jan 14, 2019

View reviewed changes

core/lib/Slots.re Outdated Show resolved Hide resolved

wokalski force-pushed the feature/hooks-api branch from 98c3183 to 12779bd Compare January 16, 2019 13:08

bryphe reviewed Jan 17, 2019

View reviewed changes

jchavarri approved these changes Jan 17, 2019

View reviewed changes

bryphe approved these changes Jan 17, 2019

View reviewed changes

wokalski and others added 9 commits January 17, 2019 18:26

Replace old ReasonReact-like API with Hooks

a0733aa

Remove stale comments

86a835c

Remove debug prints

190a2eb

Remove extraneous type annotations from Hooks

b96cb67

Add a Slots functor and allow folding with individual flushing

db4c598

This commit should also the reduce of allocations.

Improve type variable naming in Slots.rei

6cab4de

Add test case

7a64000

Add test case exercising problematic behavior in revery

3981dfa

Uncomment

d8e2388

wokalski and others added 12 commits January 17, 2019 18:26

Traverse the whole tree on flushPendingUpdates

264ff51

Add the effect hook (not hooked up to reconciliation yet)

e563319

Untested, inefficient, experimental implementation of executing hooks

5279569

Fix wrong test assertion

573d4d8

Hook up markAsStale on state updates.

16fbeba

Update mac components to the latest hook API

c06c11c

Add case for 'Always' mounting'

8ba8d2f

Add some effect test cases

0abbc80

Fix Unmount effect not being called on the top unmounted node

9e1d1ea

Add RenderedElement.executePendingEffects

1b24dce

Refactor refs to records with mutable fields

77bbe63

Rename elem to witness in Slots

c80f053

wokalski force-pushed the feature/hooks-api branch from cc24a0a to c80f053 Compare January 17, 2019 17:28

wokalski added 3 commits January 17, 2019 19:13

Fix NSView -> BriskView after a rebase

1971ddb

Rename package React -> Brisk in test/app.re

95f4312

Allow multiple subscribers for RemoteActions

e31c40f

wokalski force-pushed the feature/hooks-api branch from d755625 to e31c40f Compare January 17, 2019 19:06

wokalski merged commit e1ee4a3 into master Jan 17, 2019

wokalski deleted the feature/hooks-api branch January 17, 2019 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace old ReasonReact-like API with Hooks #27

Replace old ReasonReact-like API with Hooks #27

wokalski commented Jan 12, 2019 •

edited

Loading

jchavarri left a comment

wokalski commented Jan 12, 2019

jchavarri commented Jan 12, 2019 •

edited

Loading

wokalski commented Jan 12, 2019

jchavarri commented Jan 13, 2019

wokalski commented Jan 13, 2019 •

edited

Loading

jchavarri commented Jan 14, 2019 •

edited

Loading

wokalski commented Jan 14, 2019

jchavarri commented Jan 14, 2019

wokalski commented Jan 14, 2019

jchavarri commented Jan 14, 2019

wokalski commented Jan 16, 2019

bryphe Jan 17, 2019

wokalski Jan 17, 2019

bryphe commented Jan 17, 2019

jchavarri left a comment

jchavarri Jan 17, 2019

wokalski Jan 17, 2019

jchavarri Jan 17, 2019

wokalski Jan 17, 2019

jchavarri Jan 17, 2019

wokalski Jan 17, 2019

jchavarri Jan 17, 2019

jchavarri Jan 17, 2019

wokalski Jan 17, 2019

wokalski Jan 17, 2019

jchavarri Jan 17, 2019

wokalski Jan 17, 2019

wokalski Jan 17, 2019

Replace old ReasonReact-like API with Hooks #27

Replace old ReasonReact-like API with Hooks #27

Conversation

wokalski commented Jan 12, 2019 • edited Loading

Temporary component API

jchavarri left a comment

Choose a reason for hiding this comment

wokalski commented Jan 12, 2019

jchavarri commented Jan 12, 2019 • edited Loading

wokalski commented Jan 12, 2019

jchavarri commented Jan 13, 2019

wokalski commented Jan 13, 2019 • edited Loading

jchavarri commented Jan 14, 2019 • edited Loading

wokalski commented Jan 14, 2019

jchavarri commented Jan 14, 2019

wokalski commented Jan 14, 2019

jchavarri commented Jan 14, 2019

wokalski commented Jan 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryphe commented Jan 17, 2019

jchavarri left a 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

wokalski commented Jan 12, 2019 •

edited

Loading

jchavarri commented Jan 12, 2019 •

edited

Loading

wokalski commented Jan 13, 2019 •

edited

Loading

jchavarri commented Jan 14, 2019 •

edited

Loading