Experiment: more ergonomic/idiomatic API

[glennsl]

This aims to adapt the API to be more ergonomic and idiomatic OCaml. The purpose being to get a thorough understanding of what an API that is internally consistent with the OCaml ecosystem would look like, and what a developer coming from an OCaml background, rather than a React background, would expect. Based on this, and with the existing React-like API as a counter-point, we'll hopefully be able to come up with a synthesis that will be familiar to both React and OCaml developers, and provide a consistent experience when working with the rest of the OCaml ecosystem.

[jchavarri]

Thanks for this @glennsl, I haven't been able to review it yet, but checked the issues to unblock you.

Also, any ideas on troubleshooting hanging test issues?

I just commented all effects related tests and then uncomment progressively, until I found the culprit. I noticed that uncommenting this call to use_effect_on_change1 would trigger the problem again:

https://github.com/glennsl/jsoo-react/blob/6ffbb3bd4406b398d2cf2f2e9b537bacc6318b41/test/test_reason.re#L157

After checking the code, I saw that after the changes in this branch, the dependency for use_effect_on_change1 is now automatically wrapped in an array, in the library code:

https://github.com/glennsl/jsoo-react/blob/6ffbb3bd4406b398d2cf2f2e9b537bacc6318b41/lib/core.mli#L181

while this was not the case before:

https://github.com/glennsl/jsoo-react/blob/5b81ab32cd4fc95277da8bb2ab97842e2b7656dd/lib/core.mli#L131

Removing the wrapping in array [|f|] -> f in the dependency in the test fixes the issue:

React.use_effect_on_change1(
  () => {
    setCountStr(((count, str)) => (count + 1, f(str)));
    None;
  },
  f,
);

The same should be done for all calls to use_effect_on_change1 I believe.

[glennsl]

Ah, thanks @jchavarri. Good catch!

I think the root problem of the infinite loop in particular is a test design problem I've also come across with other tests, that they use use_state to track test state, which itself also affects the test because it triggers re-rendering. And because a new array is created for the watched variable every time, it will always be invalidated, call the effect and trigger another re-rendering etc., ad infinitum. I've converted a few tests to use an ordinary ref instead to avoid this problem, and probably should go over all the tests to do the same at some point.

[jchavarri]

And because a new array is created for the watched variable every time, it will always be invalidated, call the effect and trigger another re-rendering etc., ad infinitum.

This makes sense. It's a bit unfortunate that we have to stop using use_state in tests due to this issue.

[glennsl]

We don't necessarily have to stop using use_state, I just don't think it's a good idea to have the test setup affect the tests because it can lead to issues like this, and even obscure actual test failures.

Why do you think it's unfortunate to stop using it?

[jchavarri]

I was just thinking that the more use_state is used in tests, the better (more edge cases can be caught). But it is probably not necessary, as long as we have some specific tests for it, then it makes sense to not use it and simplify other tests like effects etc. 👍

[glennsl]

A few more adaptations made, but still a work in progress. The last commit unfortunately fails to compile with the ppx error:

Fatal error: exception (Invalid_argument
  "react.component calls can only be on function definitions or component wrappers (forward_ref, memo).")

And unfortunately I'm not able to understand the function that emits this error:

https://github.com/ml-in-barcelona/jsoo-react/blob/8805668dd8a1c64b55eecfdd1719a52c45b336f9/ppx/ppx.ml#L693-L717

Some help with this would be greatly appreciated :) (Unless this change is a really bad idea for some reason)

[jchavarri]

@glennsl this part of the ppx is quite hairy 🐻 it is the part that dives into the expression that has the react.component attribute to see what are the labelled args, so we can gather them to create the makeprops function. For the majority of cases the labelled args can be retrieved from the "top" expression (the one that has the react.component attr). However, for cases like React.memo we have to keep "digging" into the parsed tree to get the labelled args. Hence the function name spelunk_for_fun_expr. Btw most of this code comes from reason-react ppx, I barely touched it, maybe there are easier ways to handle this.

I pushed a commit that fixed ppx and tests in https://github.com/ml-in-barcelona/jsoo-react/commit/744f3697e78c96a216bb225cbc5aa49cb62347c6, feel free to pick whatever you need or improve it.

There is one thing that if you don't pick I will add in another PR though, which is the locations for the react.component calls can only be on function definitions... error. I think nobody should suffer the pain of the ppx bailing out without any information on where the error is happening 😬

[glennsl]

Thanks @jchavarri! I was completely missing the second location that had to be changed, and therefore couldn't figure out why it failed no matter how I tweaked the first pattern. Also really appreciate the better error reporting!

This mechanism does seem very fragile. But I think I improved it a little bit, with the help of having the argument labelled, by not requiring it to be a syntactic function and allowing it to be placed both before and after the unlabeled argument.

[glennsl]

I'm pretty sure I've gone all the API tweaks I wanted to make now. As explained in comments, and also now in the OP, in this first iteration I wanted to answer the question "what would this API look like if it was designed purely from an OCaml perspective?". And I think in a first round of review it would be good to take that perspective as well. Then, once we have a clear understanding of the two "extremes", their benefits and problems, we can go about trying to figure out how to make the API approachable from both directions. A few alternatives we can pursue to that end:

• Compromise the API design by trying to eliminate "problems" from both perspectives, sacrificing some of the benefits along the way.
• Keep the primary API idiomatic OCaml, but add a compatibility layer providing a more faithful and familiar React API.
• Provide specific documentation targeting React developers to explain the differences and rationale.

[glennsl]

No idea what's wrong with the Windows CI today. It seems to fail on everything.

[jchavarri]

Thanks!