Hooks using OCaml idioms and equality semantics

[glennsl]

Builds on #141, Addresses #153

This adds a use_effect hook that uses OCaml equality semantics, and partly because of that also provides a much nicer API, in my opinion.

I also added a use_ref hook using an actual ref, because I needed that for use_effect and got tired of having to dealing with React.Ref when OCaml already has a first class ref cell.

If this API style agrees with y'all too I can go on to add equivalent hooks for use_memo and use_callback. I also suppose this is a good opportunity to move the old bindings to the reason-react compatibility layer.

[jchavarri]

@glennsl I am confused by this PR, a lot of the changes were already part of #141 right?

[glennsl]

Yes, sorry, I have no idea why I based it off that. I'll rebase it shortly, but it's just the three last commits that are relevant.

[glennsl]

CI is fun, huh? Ubuntu fails almost immediately with some fetch error, Windows complains about formatting while MacOS is fine with the exact same formatting...

[jchavarri]

The ubuntu issue seems more on Github side (based on "Not Found [IP: 52.252.75.106 80]" error).

But the formatting one? maybe some bug in ocamlformat? 🤔 ocaml-ppx/ocamlformat#1773

[glennsl]

I think it's with the Ubuntu repositories. It was reported now on Discord as well. It'll probably sort itself out eventually.

The windows formatting error seems to be just with this PR, in files that it doesn't even touch. And it seems to be complaining about blank lines following end-of-line comments, which (annoyingly) is enforced by the formatter locally. I know because I tried to remove them too. Maybe by some fluke this CI box got a newer version of ocamlformat? Will probably sort itself out next round as well then.

[jchavarri]

I am not sure about the deep comparison approach, but I guess in the vast majority of cases the performance toll would not be costly 🤔

I also suppose this is a good opportunity to move the old bindings to the reason-react compatibility layer.

I am wondering if the old bindings should remain available in Core? If not, how could it be made available in reason-react compat layer without making it public?

[jchavarri]

Alternate names proposal for labelled args:

• on -> deps, dependencies or depends
• release -> cleanup (trying to follow React docs naming)

Minor, but isn't acquire param the effect itself? Would it make sense to call it just effect?

In general, I find the "acquire" / "release" terminology confusing because, unlike in RAII, here the effects don't necessarily involve any resource allocation.

[glennsl]

I'm not entirely convinced by the terminology either, but I still think I prefer the choices I made (shocking, I know!)

dependencies, or variations thereof, seems a bit vague in what the dependency relationship actually is, whereas on quite clearly alludes to the effect happening "on" those values changing. I think you'll still have to read the documentation to fully understand how this works, but once you've done so it seems to me that on will be easier to recall when both reading and writing the code. Also, dependencies being such a long word with several possible abbreviations can make it especially difficult to recall the right one.

Another alternative is watch, but I'm even less fond of that, despite it being fairly short. I think because it's even more vague.

I see where you're coming from with wanting cleanup being more general, but I'm not sure there is actually much of a need for this generality. Even the React documentation you refer to only mentions the need for resource cleanup:

Often, effects create resources that need to be cleaned up before the component leaves the screen, such as a subscription or timer ID. To do this, the function passed to useEffect may return a clean-up function.

But, perhaps more importantly, there's a subtle difference in this API where instead of returning a cleanup function from the effect, the acquire function can return a "resource", which will then be passed to the release function when it's time for cleanup. I think the current naming is much more helpful in suggesting this intended use.

Also keep in mind that it's still possible to pass a generic unit -> unit cleanup function to release. The difference is just that the naming focuses on (what I think is) the much more common use case.

[glennsl]

I split use_effect into use_effect, which has a cleanup parameter and no state passed between the effect and cleanup functions, and use_resource which behaves like before but makes its purpose even more apparent. While use_effect is technically just a thin coat of paint over use_resource, I think the distinction makes sense for pedagogical purposes and hope this addresses your concerns as well. Thanks for pointing it out!

I'm still using ~on for the dependencies though, while not perfect, is still better than the alternatives I think.

[jchavarri]

This has a lot of perf implications 😬 I am not sure how we should handle it, maybe reflect it in name with something like "deep"? e.g. deep_use_effect as opposed to "shallow" use_effect.

[glennsl]

Can you think of a use case where you would actually want a shallow comparison where the shallow and deep comparison is not exactly the same? I can't think of any. And if there isn't any I can't see the value in making a distinction.

But you bring up a great point, and I think at the very least it would be a good idea to add an extra parameter to pass in an equal function, so that it's possible to customize the comparison for performance reasons.

[glennsl]

Added an equal parameter in 5961c69

[glennsl]

I am wondering if the old bindings should remain available in Core? If not, how could it be made available in reason-react compat layer without making it public?

Good point. I wonder if virtual libraries somehow could make it possible to expose these to other "internal" libraries without making it part of the public API.

[glennsl]

I don't know why the ppx output is suddenly formatted differently on CI now... 🤷

[jchavarri]

Thanks!

[jchavarri]

I don't know why the ppx output is suddenly formatted differently on CI now... 🤷

Fixed in #163 hopefully.