search

revery-ui / reason-reactify

:rocket: Transform a mutable tree into a functional React-like API
MIT License
104 stars 8 forks source link

API: We should throw an exception when a hook is used outside of a component context #39

Open bryphe opened 5 years ago

bryphe commented 5 years ago

We don't do any checking or handling when a hook is called outside of a render function. This is simple to reproduce - just by calling useState outside of a component.

We should add a test case that exercises this, and validates an appropriate, actionable exception is thrown.

jchavarri commented 5 years ago

I have been talking a bit with @cristianoc about hooks and how their rules could be encoded in the type system, and he shared with me some time ago an exploration that involved using linear types to track usages of hooks inside the render function: https://gist.github.com/cristianoc/cef37bcfc0446da482da4723dc3319a8

@bryphe I believe Cristiano's solution was targeting the detection of usages of hooks inside conditional statements. But I think it would solve the problem you mention above (usages outside render).

One downside of this approach is in "API costs": the value that contains the linearity of hooks needs to be carried over between hooks function calls, and also passed to the render function.

@bryphe I wonder what you think about this, and if it would be an approach worth exploring? An implementation in the type system has some implications, but I think it could bring a better dev experience. The implementation also would be more idiomatic than having these checks implemented with tests + runtime exceptions.

I also wonder if, considering the current design of reason-reactify there could be a way to hide the hooks value from final users, so the linearity is checked internally, kind of like how we passed the context to every dispatch without end users knowing.

bryphe commented 5 years ago

Cool, thanks for thinking about this @jchavarri & @cristianoc . The current model we have in this project doesn't sit right with me, just because it's too easy to get wrong and crash. We should be 'using the language' for this.

@bryphe I wonder what you think about this, and if it would be an approach worth exploring?

I really like @cristianoc 's proposal. Preserving the hooks between function calls seems reasonable to me, because it provides a clear benefit - we can leverage the type checker.

One of the major benefits of hooks, imo, is the ability to compose & reuse them - so it was helpful to see the example in the gist of how that would work.

An implementation in the type system has some implications, but I think it could bring a better dev experience.

Agreed!

So if we used a model like @cristianoc 's - we'd get better type checking in general, and it would solve this problem (actually, completely sidestep the problem) - with the minor cost of passing a hooks around (which can be easily justified). I'm on board!

jchavarri commented 5 years ago

I played a bit with the "hooks carrying" solution, and one small downside is that it doesn't prevent users from not passing the hooks value around, in which case the checks would not apply.

For example, in this gist the last function calls a different number of times the useState hook depending on the branch, and it compiles just fine.

This makes me wonder if we should explore an approach that combines the ideas from the linear types solution by @cristianoc but changing the API so the hooks return the values inside a callback, in a continuation passing style. Essentially, the idea expressed in https://paulgray.net/an-alternative-design-for-hooks/.

This, combined with something like https://github.com/jaredly/let-anything/ could lead to a result like:

let render = () => {
  let.Hook (count, dispatch) = useReducer(reducer, 0);

  <view>
    <button title="Decrement" onPress={() => dispatch(Decrement)} />
    <text> {"Counter: " ++ str(count)} </text>
    <button title="Increment" onPress={() => dispatch(Increment)} />
  </view>;
};

This approach would remove the need of passing the hooks value to the render function, and passing it around. The syntax sugar allows to avoid all the nesting, and the result is as expressive as the JS implementation I believe. 🤔

bryphe commented 5 years ago

I played a bit with the "hooks carrying" solution, and one small downside is that it doesn't prevent users from not passing the hooks value around, in which case the checks would not apply.

Ah, good catch! That would make it easy to lose the safety net we constructed around this concept.

This makes me wonder if we should explore an approach that combines the ideas from the linear types solution by @cristianoc but changing the API so the hooks return the values inside a callback, in a continuation passing style. Essentially, the idea expressed in https://paulgray.net/an-alternative-design-for-hooks/.

Great idea! The issue I had with the proposal in the article was the syntax being more cumbersome to work with (for a JS dev coming from React), and I wasn't sure how to address that. But your idea to use a PPX solves that!

I think the the let.Hook ... is very reasonable, to get the benefit of the type safety. We'd essentially just add one rule to our Rules of Hooks - use let.Hook for hooks. Benefit - you get type safety, and you might not have to care as much about the other hook rules, because we'd help enforce it for you with the type system. It's a very small tradeoff for all the value you'd get from the type system!

Do you see any downsides with the let.Hook approach, @jchavarri ? It seems like custom hooks would be supported - we'd just use let.Hook inside those too, correct?

It's pretty amazing to me that we could validate the situation you mentioned - calling useState multiple times - in such a seamless way. Really cool stuff 😄

cristianoc commented 5 years ago

This looks like a very interesting direction. @jchavarri would you flesh out what let.Hook would desugar to?

jchavarri commented 5 years ago

@bryphe Glad you're on board with the idea! 😄

We'd essentially just add one rule to our Rules of Hooks - use let.Hook for hooks.

I think with a type-based solution, there would be no more need for explicit rules anymore... one would just have to follow the type system guidance (but any additional documentation to help understand what the type system is doing always helps 🙂 ).

Do you see any downsides with the let.Hook approach, @jchavarri ? It seems like custom hooks would be supported - we'd just use let.Hook inside those too, correct?

One downside imo is that it's a heavier approach than the current API. So to alleviate the heavier API we introduce a dependency on the PPX, which is also not ideal as ppxs bring some inherent complexity. But with native support for the continuation passing style ppx in Reason in the horizon (https://github.com/facebook/reason/pull/2140) I see the risk of that dependency would be kind of mitigated.

The other downside is that I think the more complex types make the API less accessible. While this kind of abstractions are quite idiomatic in OCaml and other typed functional languages like Haskell, I think it takes some time for someone coming from JavaScript to understand them because there's no referents to rely upon in the language we come from. Or at least that's been my experience. 😅 It took me some time for these monadic solutions to "click" for me, and I still struggle when I have to design or work with monadic-like types. Using ppxs also obscure the dev experience, as errors can be more cryptic etc. However, I think these challenges can be counter-balanced with great documentation and lots of good examples. I believe that the unlocked power that the type system offers makes it worth it!

@jchavarri would you flesh out what let.Hook would desugar to?

@cristianoc Sure! The example above would desugar to:

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

In terms of types, I'm not sure yet about all the details and would appreciate any insights you might have, but it seems the return value of the components render function would go from simple type element to an abstract type element('t) where 't "accumulates" the different types of all the core hooks that are called inside:

module Hooks = {
  type state('a);
  type element('t);

  let primitive: string => element(unit) = Obj.magic;

  let addState:
    (~state: 'state, element('t)) => element(('t, state('state))) =
    (~state as _, x) => Obj.magic(x);
};

let useState = (state, f) => {
  let setState = x => ignore(x == state);
  Hooks.addState(~state, f(state, setState));
};

let helloComponent = (~message) =>
  useState("Harry", (name, _setName) => {
    Hooks.primitive(name ++ ", you have a message: " ++ message);
  });

In that case, helloComponent has type:

(~message: string) => Hooks.element( (unit, Hooks.state(string) ) )

The custom hooks, because they're made from core hooks, would also compose the core types accordingly. I think / hope 😄

jchavarri commented 5 years ago

@bryphe Another downside: there is an extra function call that has to be made to call the callback passed to the hook to get the results back. I mention this not only for perf (the cost of a function call can probably be omitted) but mainly for debugging purposes. From Dan Abramov's blog post:

Another way to impose more ceremony is by making Hooks monadic or adding a first-class concept like React.createHook(). Aside from the runtime overhead, any solution that adds wrappers loses a huge benefit of using plain functions: they are as easy to debug as it gets.

Plain functions let you step in and out with a debugger without any library code in the middle, and see exactly how values flow inside your component body. Indirections make this difficult. Solutions similar in spirit to either higher-order components (“decorator” Hooks) or render props (e.g. adopt proposal or yielding from generators) suffer from the same problem. Indirections also complicate static typing.

Except for the last sentence –I don't understand exactly what he meant with "indirections" and "complicate"– I can see the points he makes.

There was a very interesting convo on Twitter a few months ago about a monadic solution for React hooks that mentioned some of the downsides and upsides of a monadic solution: https://twitter.com/rauchg/status/1057662611641196544

jchavarri commented 5 years ago

I started something in https://github.com/jchavarri/reason-reactify/commit/02b8643d0a10d1367a57a60410a336e9a7db889c.

I'm struggling with some typing issues, because the callback in useState returns a type hook(state('a)). This goes against all the current type assumptions, where all render functions return a component, the children are a list(component) etc.

So, right now, the compiler fails to type check.

I wonder if there's a way to keep the simple type component around, but still have the guarantees that the abstract type hooks('a) provides. I was thinking on adding some conversion in this line:

https://github.com/jchavarri/reason-reactify/blob/02b8643d0a10d1367a57a60410a336e9a7db889c/lib/Reactify.re#L141

But I can't come up with anything that works. Maybe we actually need to propagate the hooks('a) type all around? 🤔

bryphe commented 5 years ago

Thanks for the thorough and detailed proposal @jchavarri ! Those twitter conversations on hooks / type safety were really interesting. I suspect that perhaps the monadic / continuation models in JavaScript have a different trade-off profile vs here in Reason - since there are lots of JS environments that couldn't give you type-safety (and no ubiquitous tooling like ppx), it's not worth it. However, since we do have those tools with reason, and the ppx transform lessens the cost of the continuation approach - having the type-safety seems totally worth it! It sounds like @jordwalke was on board with a plan like this from the twitter thread.

I'm struggling with some typing issues, because the callback in useState returns a type hook(state('a)). This goes against all the current type assumptions, where all render functions return a component, the children are a list(component) etc.

Yes, this seems tricky! It seems like the important place for this is the place we actually call the component's rendering function, the c() here:

      Component(
        id,
        () => {
          Effects.resetEffects(__globalEffects);
          let children: list(component) = [c()];
          let effects = Effects.getEffects(__globalEffects);
          let renderResult: elementWithChildren = (
            children,
            effects,
            __globalContext^,
          );
          renderResult;
        },
      );

I wonder if there is a composite type we could return here, but then discard the hook(state('a))? I don't believe we have a need for it in the internals, as it is primarily for verifying the semantics of the hooks usage in the component's rendering function.

Thinking about a desugared useState example:

let render = () =>
  useState(0, (state, setState) =>
    <view>
      <text> {"State: " ++ str(count)} </text>
    </view>
  );

Perhaps it would make sense for useState to return a tuple of (list(component), hook(state('a)). Then, we'd update our children as something like:

let childrenAndHookState = c();
let (children, _) = childrenAndHookState;

(Something like that). I believe this would keep us from needing to propagate the hook(state('a)) everywhere. We'd basically use the type accumulation in the render method to ensure consistency, but then scrap it internally once we get the list of children.

I'm not an expert on managing the type system though; not 100% sure this works! I'm optimistic there is a way to express this though.

Maybe we actually need to propagate the hooks('a) type all around

Hmm, what would this look like? Would we need to add hooks('a) to the component type?

jchavarri commented 5 years ago

Thanks for the help and guidance @bryphe ! I really appreciate it. 🙂

I'm documenting here the progress so it serves to arrange my thoughts, and maybe also helps in case you, @cristianoc or maybe others have ideas on how to keep moving forward, as I'm not sure I have enough knowledge about OCaml and type systems to implement this 😄 .

I started following the path you proposed: make useState return a tuple. However, that change causes a cascading effect in the types design:

But in that last step is where I find the main show-stopper. We don't want to consolidate all children under the same type, as we want to keep allowing components to render one child with useState, another without any hooks, another with useState and useReducer, etc.

So it seems we might want an approach without tuples.

Then I thought about a different strategy which would involve adding an extra variant to the component type:

type component =
    | Primitive(primitives, render)
    | Component(ComponentId.t, render)
    | Provider(render)
    | Empty(render)
    | Hook(hookChain, component)

With hookChain being the type keeping track of the hooks chaining. However, it would have the same problem: hookChain would need to be made abstract (hookChain('t)), so that type variable would "propagate" upstream to all functions using the component type.

So... maybe making hookChain a GADT, so the type variable doesn't escape to the wrapping component type could be a potential path to follow? I'm not 100% sure how to do this 😅

To summarize, I think my issue is I don't know how to create a type for hooks that is composable / abstract so we can keep track of the different hooks that are applied in the component render function, and at the same time without leaking that type variable across other types that would be forced to carry that variable around.

I hope this makes some sense! 😄

bryphe commented 5 years ago

Thanks for the help and guidance @bryphe ! I really appreciate it. 🙂

Not sure I'm actually helping at all, I'm learning right along with you 😄

Great write-up and summary as always!

I was reading through this blog post again - Diff Lists - trying to get some ideas. I'm still trying to learn GADTs so I hope that everytime I read it maybe more will sink in 😄 It describes an interesting way to construct type-safe, heterogenous lists.

So... maybe making hookChain a GADT, so the type variable doesn't escape to the wrapping component type could be a potential path to follow? I'm not 100% sure how to do this 😅

At the top of that blog post - there is an example of creating a heterogenous list which has a hidden type (using an existential). The syntax is like this (in reason):

type ex_list =
  | Nil: ex_list
  | /** 'a is hidden! */ Cons('a, ex_list): ex_list;

I suspect we could use a similiar strategy to drop the type variable for the hookChain, making it abstract. That might be enough for us to get away with making this work, since, we could use the existing scaffolding today w/o reading from the hookChain. That perhaps may be enough to allow us to get to the next step of prototyping this approach. If I understand correctly, it gives us a way to 'hide' that type variable so it doesn't escape or propagate.

If it does - I wonder then if we could extend that 'diff list' idea in the blog post, and instead of a tuple of (hookChain, component) we could create a heterogeneous linked-list of hook 'effects' to model the rendering?

My thinking is we could have a hook type:

type component =
    | Primitive(primitives, render)
    | Component(ComponentId.t, render)
    | Provider(render)
    | Empty(render) 
and hook
/* | UseEffect(...) */
| SetState(state)
| Render(component);

We'd be generalizing the actual component-render as a hook too here. In other words, components would always return a list of hooks - but in the normal-no-hook-case, it would just be a single node of Render(component). As you chain hooks, it'd create longer lists, up to the final Render(component). The diff list concept in the blog could help us by maintaining the typing across that linked list.

I was playing with this idea and it would certainly have downstream impacts (didn't quite get it compiling, yet) - https://github.com/revery-ui/reason-reactify/tree/bryphe/wip/ideas-for-hooks - it would unfortunately involve cross-cutting changes - every component would need to now render a 'hook', even if it is just wrapping their existing behavior.

Then, there'd be a couple cases:

We'd always return this typed, heterogenous linked list. The empty case would just be a single-entry, render component case. The hooks cases would just be a chain of hooks, prior to the ultimate render, like:

SetState -> SetState -> SetState -> Render

The chaining would happen naturally as a result of your proposed continuation model - we'd append a node to the list at each level of the continuation, until we returned the full diff-list of hooks + render.

One nice property of this is that (I think) we could get out of the business of having to track global state for effects / hooks. The render function would return this linked list / diff list of 'hooks' from the render function, which conveniently stores the same things that we currently put in our global state management / effect management variables.

We could then unpack the resulting linked-list it to get the child components (it'd be a Render(component) entry at either the head or tail of the list, depending on how we order it), for reconciliation, as well as store the linked list on the instance to preserve the state. Since the order is always the same - rehydrating the state for an instance would be just traversing the list in the right direction.

Sorry this is sort of a brain-dump and nothing concrete... just some ideas I had trying out the diff list idea.

jchavarri commented 5 years ago

Thanks a ton @bryphe !!

Your message is very encouraging because I was doing some work in parallel: https://github.com/revery-ui/reason-reactify/compare/master...jchavarri:starting-hooks?expand=1

And it seems both approaches are quite similar 🎉 as they're both based in GADTs. I'm also learning GADTs, but from what I understand, we can leverage them to keep the abstract types coming from the different hooks (like (reducer(string), state(int)) hidden temporarily as they get "packed" into the GADT, which makes life easier for intermediate parts that don't necessarily need to know about the internals of these abstract types.

In the branch starting-hooks, I'm creating a new type wrappedElement:

and wrappedElement =
    | Hook(component, hook('t)): wrappedElement
    | Regular(component): wrappedElement

which is very similar to the hook type in your branch:

and hook =
    | SetState('a): hook
    | Render(component): hook

(Side note: I think we should rename the type component to element for clarity, as its meaning is closer to the second, but I'm curious what your thoughts are...)

One of the things I'd like to try is to keep the useState and other hooks calls as merely abstract types, nested into each other through tuple types. The benefit of this approach (as opposed to represent them with a list of variant types) is that it has zero runtime overhead, as all these types gets erased once one "unpacks" the GADT and discards the part that represents the hooks nesting. I'm not sure if it's totally possible but from Cristiano's example it seemed a possibility.

So, a component with hooks would always return Hook(component, hooks('t)) with the hooks('t) never becoming a value that is realized at runtime, but just existing at compilation type with types like hook((state(int), hook(reducer(int))) etc.

@bryphe What do you think about the approach above? Maybe it's over complicating things a bit? I'm not sure where these roads will take us but in general using a GADT seems like a promising path 😄 Will keep investigating!

jchavarri commented 5 years ago

Hm... so, after more attempts in the starting-hooks branch, I'm not sure I know how to make this approach work with the first class modules.

The challenge is that we want the calls to useState and other hooks to return a different type depending on the hooks used by each component. In particular, depending on the type of hook + type of the state. This unfortunately doesn't go well with the GADT approach to "hide" the type we were discussing above: if we hide the hooks types, then all hooks would return the same type and thus no errors would be shown by the compiler if some component uses different hooks in the render function depending on some condition.

So, let's assume, supposing the above is true, that the component module contains a type t('t) instead of a simple t to reflect the dynamic type returned by the render function. Then I'm afraid we can't keep using first class modules as they don't allow to abstract over parametric types. We might need to use functors for this (see https://discuss.ocaml.org/t/functors-vs-first-class-modules/1722)

@bryphe @cristianoc Does the above make sense? I might keep exploring the functor path, unless you know if there are ways to implement this behavior using first class modules?

cristianoc commented 5 years ago

@jchavarri I had the same thought: that with GADTs one can abstract only at the end.

bryphe commented 5 years ago

The benefit of this approach (as opposed to represent them with a list of variant types) is that it has zero runtime overhead, as all these types gets erased once one "unpacks" the GADT and discards the part that represents the hooks nesting. I'm not sure if it's totally possible but from Cristiano's example it seemed a possibility.

@jchavarri - this sounds like a very promising approach! Essentially compile-time magic with the type system that has no runtime impact, but validates at compile-time that the hooks are used correctly 👍 And then as @cristianoc mentioned - we can abstract at the end so that it doesn't need to percolate through the rest of the types.

(Side note: I think we should rename the type component to element for clarity, as its meaning is closer to the second, but I'm curious what your thoughts are...)

Definitely open to it! Being crisp on the nomenclature (component, element, instance, primitive) will go a long way in helping the code be more understandable. Like an issue I ran into with component is it was both a type and the name of that function that returns first-class modules - confusing.

Then I'm afraid we can't keep using first class modules as they don't allow to abstract over parametric types. We might need to use functors for this (see https://discuss.ocaml.org/t/functors-vs-first-class-modules/1722)

I'm not coupled in any way to our implementation using first-class modules - so I'm happy to go down the functor route instead. I view what we have with first-class modules just being a stepping-stone to a nicer API, and I think things like the val keyword and the confusing render interaction will be confusing to newcomers - so certainly open to improvements!

Your PPX prototype showed some interesting possibilities in creating modules under-the-hood of a functional model - so I imagine that combined with this functor route could lead to some really interesting and streamlined syntax possibilities! 🎉

jchavarri commented 5 years ago

@bryphe Thanks for all the feedback! It's really great to know there's room to explore, and that these proposals can be considered and might be ultimately used 🙂

@bryphe @cristianoc I continued exploring / learning more about functors, first-class modules and type unification. After trying a few solutions based in functors, I realized there was no need for them if we used the same signature that we use now, where the component function gets a "continuation-styled" callback as param, where render is injected as the first parameter. I took advantage of this design to introduce the "de-hookification" as part of that render function, and it seems to please the compiler.

I have created a sketch with all the prototyping code and some details about the latest state of the solution: https://sketch.sh/s/3SQyO9p2IXgm0mEBPNiKXW/

Note that that approach is probably too generic: there are 3 types injected in the createComponent function to the Component module, while in revery I think we will need only 2. 🤔

If you're on board with that solution, I can start trying to implement it in a new PR. I have a couple of thoughts / questions:

module IntComponent = (
  val createComponent((render, {useState}, ~one, ~two) =>
        render(
          useState(3, (state, _setState) => <text>{string_of_int(one + two + state)}</text>),
        )
      )
);

Maybe there could two factory functions: createComponent and createHookedComponent. I wonder how a let%component ppx would play with this injection as well.

In any case, I think that last one is a smaller change, and I would probably tackle the "continuation-passing" API for hooks first, and we can always figure this out later on. I also wonder how this new param.

What do you think?

jchavarri commented 5 years ago

I got the continuation-passing style working in https://github.com/revery-ui/reason-reactify/pull/43 🎉 And I'm exploring a ppx in the hooks-ppx branch.

Very exciting! 😄

lethook

jchavarri commented 5 years ago

To go back to this original issue (sorry to derail a bit from it 😅 ) I have been thinking how to prevent usages of hooks outside the render function.

I'd like to propose to add the hooks to the callback that is used in the component function (renamed to createComponent in #43), instead of being exposed as direct values from the result of the Reactify.Make functor.

So, instead of:

module ComponentWithState = (
  val createComponent((render, ~children, ()) =>
        render(
          () => useState(2, ((s, _setS)) => <aComponent testVal=s />),
          ~children,
        )
      )
);

We would do:

module ComponentWithState = (
  val createComponent(({render, useState}, ~children, ()) =>
        render(
          () => useState(2, ((s, _setS)) => <aComponent testVal=s />),
          ~children,
        )
      )
);

The first param of the function passed to createComponent would become a record that contains the render function + all the hooks functions.

Upsides

Downsides

@bryphe If you're on board with this, I could start working on this now, so maybe can get added to 3.0.0 so you don't have to publish too many new versions. It should be a pretty straightforward change 🤔 (famous last words...) 😄