Usages of `concat`/`append`/etc conflict between `JsArray` and `Array`

[amsross]

There are places in bastet_js/src/JsArray and bastet/src/Array that use Js.Array.concat and ArrayLabels.concat/ArrayLabels.append respectively. The behavior of these functions yields different results, though, so it looks like each module would not pass the tests of the other.

eg:

JsArray.Monad.flat_map([|1, 2|], x => [|x, x|]) == [|2, 2, 1, 1|];
Array.Monad.flat_map([|1, 2|], x => [|x, x|]) == [|1, 1, 2, 2|];

JsArray.Unfoldable.unfold(x => x > 5 ? None : Some((x, x + 1)), 0) == [|5, 4, 3, 2, 1, 0|];
Array.Unfoldable.unfold(x => x > 5 ? None : Some((x, x + 1)), 0) == [|0, 1, 2, 3, 4, 5|];

Is this desired?

[Risto-Stevcev]

Ah no, it needs to be flipped like

  let alt = (a, b) => ArrayLabels.append(a, b);

for Array as well. I'll push up a fix

[Risto-Stevcev]

You were using a slightly older version -- this was resolved in the 1.2.2 release from #23 I'll add some tests to avoid any confusion

[amsross]

23 fixed the issue for Alt but not Monad or Unfoldable. The code I pasted above is from 1.2.3.

[Risto-Stevcev]

Would you be willing to submit a PR for those fixes? I'm not sure when I'd get around to it tbh

[amsross]

Definitely; I'm throwing one up right now.

I'd love to add a test for clarity/surety but the difference between how bastet/src/Test.re and bastet_js/src/Test_JsArray.re work is eluding me.

[Risto-Stevcev]

You can look at my last commit on how to do it, you can just copy pasta that and change the assertion. bastet/src/Test.re is a functor that accepts any generic unit and generative test framework. bastet_js/test/Test_JsArray just runs it using mocha and jsverify (native runs on alcotest and qcheck) No worries though if you don't feel like it

[Risto-Stevcev]

Actually don't worry about adding tests -- I think the best solution might be to get Monad and Unfoldable to use the Array.alt function directly instead of ArrayLabels (you'd need to move Alt to be before Monad. That way code is never out of sync and alt is already tested (same should probably be done for List)