reason-reactify icon indicating copy to clipboard operation
reason-reactify copied to clipboard

Hooks with slots

Open jchavarri opened this issue 5 years ago • 2 comments

🚧 🚧 🚧 --- WIP --- 🚧 🚧 🚧 🚧

Implements the ideas in #47.

API changes

The main public API change involves the passing of slots instead of unit to the inner function inside createComponent, like:

let renderCounter = slots => {
  let ((count, dispatch), _slots) = slots |> useReducer(reducer, 0);
  <view>
    <button title="Decrement" onPress={() => dispatch(Decrement)} />
    <text> {"Counter: " ++ str(count)} </text>
    <button title="Increment" onPress={() => dispatch(Increment)} />
  </view>;
};

module CounterButtons = (
  val createComponent((render, ~children, ()) =>
        render(renderCounter, ~children)
      )
);

TODO:

  • [x] Add new ReactifyExperiment.re file, in parallel to the existing one (given there will be a lot of changes)
  • [x] Remove previous hook('t) types
  • [x] Add initial version of Slots module
  • [x] Apply slots approach to useReducer
  • [ ] Figure out reconciliation when setting state after a call to useReducer
  • [x] Apply slots approach to useState
  • [ ] Figure out reconciliation when setting state after a call to useState
  • [ ] Apply slots approach to useContext
  • [ ] Apply slots approach to useEffect

jchavarri avatar Jan 08 '19 01:01 jchavarri

Figure out reconciliation when setting state after a call to useReducer

@bryphe Regarding the above, I am investigating how to trigger a reconciliation after setting the state. Before, we relied on the instance stored in the context but that could potentially be gone now.

I have tried a very naive approach where I'd pass all the instance info together with the slots, so that could be picked up transparently:

https://github.com/jchavarri/reason-reactify/blob/98e025b32c900bdf889822005053ed2c36f8a407/lib/ReactifyExperimental.re#L525

However, this doesn't work because we would need to get the produced instance before it's been produced. There are workarounds but that seems like a wrong path.

Instead, I was thinking a couple of things:

  • Could we trigger the reconcile operation on the container of the instance where the state update happened? That would simplify things, but I'm not sure if it could have consequences
  • Would it make sense to explore an approach like ReactMini, where updates are queued and processed in one step? I guess that with the current heterogeneous approach with tuples, these updates would be trickier to implement 🤔 cc @cristianoc

Curious to know your thoughts! 🙂

jchavarri avatar Jan 08 '19 01:01 jchavarri

Awesome @jchavarri ! So excited to see this come together! 😍

I like the side-by-side approach with ReactifyExperiment you took to iterate on the new API quickly.

I have tried a very naive approach where I'd pass all the instance info together with the slots, so that could be picked up transparently:

Ah yes - was thinking about this too when I read through the issue. It does seem like it would be challenging to get this right. Agree that it seems like the wrong path.

Could we trigger the reconcile operation on the container of the instance where the state update happened? That would simplify things, but I'm not sure if it could have consequences

This seems reasonable to unblock the API. The downside is it would mean a (potential) perf hit, if we end up needing to reconcile more of the tree than before. For example - if you have a large set of rendered elements, and a leaf node calls setState, it would be ideal if only that subtree needed to be reconciled.

However, without benchmarks, it's hard to draw a conclusion, and it could very well be a premature optimization in the native world. The nice property of this is that #44 would be fixed with this too, I believe!

Would it make sense to explore an approach like ReactMini, where updates are queued and processed in one step? I guess that with the current heterogeneous approach with tuples, these updates would be trickier to implement 🤔 cc @cristianoc

I do like the model in ReactMini! It seems very efficient. But I also am not sure how to map this strategy into the slots world. If @cristianoc has ideas on how to made it work in the slots world - then it seems like it would potentially perform better than the previous strategy.

If we can't find a good way to do the queued updates - I'm on board with the full reconciliation. This change would bring us much closer to our desired functional component + type-safe hooks API surface, and then we can later focus on the internals, if we decide we need to squeeze more perf out (and when we have proper benchmarking in place, to know exactly where to zero-in). revery and Oni2 will give us some good real-world scenarios to test performance down the road, too 😎

Great progress!

bryphe avatar Jan 08 '19 03:01 bryphe