OPAM: upgrade Js_of_ocaml to 5.1.0

[zbaylin]

5.0.0 was previously "supported", and it doesn't look like there are any breaking changes in 5.1.0.

It does look like there might be an issue with using jsoo-react with OCaml 5 and --effects with JSOO, but the OPAM file restricts the compiler be between 4.12 and 5.0 (exclusive), so that can be fixed in another PR.

[zbaylin]

Looks like I spoke too soon -- I'll investigate the use_memo failures.

[zbaylin]

@davesnx @jchavarri I took a look at the useMemo1 tests and I think now that OCaml strings are just Strings in Javascript, this line: https://github.com/ml-in-barcelona/jsoo-react/blob/0b54a379bb034321a43de2d6a8773ae0212d5814/test/test_ml.ml#L587 (and the similar one in the Reason tests) do not cause useMemo to re-compute the function. This actually seems reasonable to me, but could be considered a "breaking change" -- how should I proceed here? If I change "foo" to "bar" in that line, the tests pass as expected, but maybe there was some other behavior you were trying to test that I'm not seeing? Regardless, let me know and I'll make the requisite changes.

[davesnx]

Haven't looked too much into this, but that's actually a bug fix @zbaylin. Probably worth increasing the versioning thought since 5.x can bring more problems similar to this one.

[jchavarri]

Hm I see, it seems the "problem" is that some renders of a given component that would not be memoized before are memoized now right?

I am not sure if it's better to increase versioning, but it doesn't hurt.

how should I proceed here? If I change "foo" to "bar" in that line, the tests pass as expected, but maybe there was some other behavior you were trying to test that I'm not seeing? Regardless, let me know and I'll make the requisite changes.

I think changing to "bar" works. Another idea would be to use a different data type (e.g. lists) to show how memoization works, maybe that would make the tests more obvious.

The original idea of the test was to show that memoization would work only if you pass the same reference. But if you pass the string "foo" twice as constant, the render would not be memoized because both strings were separate objects in memory. But that's not the case anymore after the memory representation changes.

[zbaylin]

The original idea of the test was to show that memoization would work only if you pass the same reference. But if you pass the string "foo" twice as constant, the render would not be memoized because both strings were separate objects in memory. But that's not the case anymore after the memory representation changes.

Yeah this is my theory as well, since

> { "foo": "bar" } == { "foo": "bar" }
false
> "foobar" == "foobar"
true

I will change the test to use bar -- In general I think this just shows that consumers of the library have to care about the JS representation of their values, but that was always true.

[zbaylin]

Hey guys, look like the tests pass now & CI is happy -- lmk if there's anything else you want me to do to get this merged.

[jchavarri]

@zbaylin thanks!