search

revery-ui / reason-reactify

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

Encode "Rules of Hooks" in the API / type system. Rule 1 of 2: "Only Call Hooks at the Top Level" #43

Closed jchavarri closed 5 years ago

jchavarri commented 5 years ago

⚠️ Warning: Breaks public API ⚠️

Breaking API changes:

Related to https://github.com/revery-ui/reason-reactify/issues/39. It doesn't fix that issue though, as it's still definitely allowed to call useState from anywhere in the code.

Why

The main idea behind this PR is to encode part of the "Rules of Hooks" in the type system + API. In this case, the rule targeted is "Only Call Hooks at the Top Level". After this PR, using a hook in one branch of a conditional expression will fail to compile. One can still decide to call hooks inside conditionals statements, as long as the types of these branches coincide.

In a future PR, we could tackle the second rule: "Only Call Hooks from React Functions", by exposing the hooks functions (useState, useReducer etc) from inside the createComponent callback.

What

This PR introduces a new polymorphic type hook('t) that allows to represent the different hooks that are used inside a component render function. Besides the new type hook('t), there is one new type for each basic hook: effect, state('s), and reducer('s, 'a).

TODO

bryphe commented 5 years ago

@jchavarri - looks great to me! Thank you for the work on this, it's incredible!

The main idea behind this PR is to encode part of the "Rules of Hooks" in the type system + API. In this case, the rule targeted is "Only Call Hooks at the Top Level".

Still amazed that the ML type-system can represent and express this.

Adapt useContext + examples (should we change this one for consistency? or leave it like it is)

I'm ok deferring this for now (or in a separate PR). It probably makes sense to change this one too, eventually, but it isn't as important as useEffect/useState/useReducer since it has some basic type safety and isn't impacted by the order of the others.

Create a ppx let%hook to make continuation-passing style consumption easier (maybe in a new PR?)

🎉 🎉 🎉 Can't wait! I think it makes sense to move it to a separate PR, though - I don't want you to have to deal with merge conflicts if the tests change or anything - it's already heroic work.

jchavarri commented 5 years ago

Thanks @bryphe !

I'm ok deferring this for now (or in a separate PR). It probably makes sense to change this one too, eventually, but it isn't as important as useEffect/useState/useReducer since it has some basic type safety and isn't impacted by the order of the others.

I ended up adding useContext to this PR, to avoid an extra breaking change later on.

I also added a small CHANGELOG.md file in the root folder to keep track of these changes, it is quite useful for visitors / users imo but I wonder what you think? It's lacking a ton of previous changes because I'm not aware of everything that happened in the past and when, but feel free to adapt / expand on it if you think it's a good idea 🙂

@bryphe I would feel more comfortable if you could take a last look to the PR to review the latest changes (just the last 2 commits). If you think it's ready, please feel free to merge or I can do it after of course. Thanks!

bryphe commented 5 years ago

I ended up adding useContext to this PR, to avoid an extra breaking change later on.

Cool! I looked over this change, looks perfect to me 👍 It is nice to not require a breaking change down the road.

I also added a small CHANGELOG.md file in the root folder to keep track of these changes, it is quite useful for visitors / users imo but I wonder what you think?

Great idea!

bryphe commented 5 years ago

Merging, incredible work @jchavarri !