raquo / Airstream

State propagation and event streams with mandatory ownership and no glitches
MIT License
246 stars 28 forks source link

New: `.splitByIndex` and `.splitOption` for vars #129

Closed phfroidmont closed 2 weeks ago

phfroidmont commented 3 months ago

Here is my attempt at adding some common operators for Var. I mostly copied and adapted what was already done for Signal. The tests are very much a copy of the Signal equivalent. I'd be happy to add more if you want to cover more specific cases.

raquo commented 3 months ago

Thanks, looks good! 👍

The existing Var split tests in Airstreamn only covered the reading (signal) part, but for var splitting we should also cover the writing part, i.e. verify that updating those derived vars actually works as expected.

This is rather annoying to do in Airstream without access to Laminar's lifecycle management, so for regular Var splitting I implemented those writing tests in the Laminar repo. If you could look at the "standard child Var behaviour" test in Laminar's SplitVarSpec.scala, and add a similar one for splitByIndex (yes, to the same SplitVarSpec.scsla test file in Laminar repo), that would be great.

Don't worry about other tests in that file, no need to re-test those things for splitByIndex.

Basically what that "standard child Var behaviour" test is doing, is splitting the var, and then inside of it, creates child elements that record a bunch of events into the log when they're created/updated/etc., then we verify that the expected events were logged, then we simulate user clicks on those child elements (clickOnItem), which triggers Var updates (via the split mechanism that we want to test, so that's the important part), then we again test that the DOM looks as we expect, and that the expected events happened to achieve that.

So basically you would take that one test, duplicate it, replace split with splitByIndex and I guess it's mostly the same after that, you'll just be using the index as the new id (make sure to rename references to id as index where it makes sense).

OTOH I think we can skip also rewriting that test for splitOption: compared to splitByIndex, its implementation is much more obvious, AND the test would need more changes, so less ROI on that – but if you feel like it, go for it.

raquo commented 3 months ago

PS Running the test in the Laminar repo would require publishLocal-ing your new version of Airstream – so your tests will pass locally, but will fail in CI, but that's fine. Please do not include the Versions.scala file with your local version, I'll just remember that it's needed.

phfroidmont commented 3 months ago

Thank you for the all the explanations. Testing this using Laminar is indeed much easier. I ended up covering .splitOption as well since it wasn't much work.