rjaros / kvision

Object oriented web framework for Kotlin/JS
https://kvision.io
MIT License
1.2k stars 67 forks source link

table, bindEach & row - Elements do not get removed from DOM #456

Closed simwagner closed 1 year ago

simwagner commented 1 year ago

Hello there!

I've recently stumbled across bindEach() behaviour concerning tables (not Tabulator, but plain old HTML tables) in conjunction with rows that I initially wasn't expecting.

I think the best way to show what I am referring to is with a simple example: Let's assume for a moment that we've got an obervable list of Strings in our application, that gets updated from time to time. The contents of this list (i.e. the individual strings) should be displayed inside a single-column table.

The list is instantiated as usual:

val demoList = observableListOf("ENTRY 1", "ENTRY 2", "ENTRY 3")

For drawing the actual table, a Table is created and the observable list is bound to this table using bindEach(). This may look like this:

table(listOf("Column 1")) {
    caption = "Example Table"

    bindEach(demoList) {
        row {
            cell {
                span(it)
            }
        }
    }
}

At first glance, this seems to work. The table is created and contains all three elements that have been defined previously in the demoList:

grafik

Where things go wrong, however is, when elements are removed from the demoList. As an example, consider the following coroutine:

CoroutineScope(Dispatchers.Main).launch {
    delay(500)
    demoList.remove("ENTRY 3")
    delay(500)
    demoList.remove("ENTRY 2")
    delay(500)
    demoList.add("ENTRY 4")
    demoList.add("ENTRY 5")
}

This coroutine initially removes two of the already existing entries in the list. Afterwards, two new ones are added. Personally, I would have expected the resulting table to contain three entries: ENTRY 1, ENTRY 4 and ENTRY 5. However, that's not what actually happens. In this example, ENTRY 2 and ENTRY 3 actually stay in the DOM, despite having been removed from the list.

We then end up with this: grafik

The resulting DOM looks like this: grafik

Whats pecuiliar in particular: This behaviour seems to be limited to the use of the row element within the table's bindEach(). For testing, I've replaced the row-instantiation with simple div and p tags.

table(listOf("Column 1")) {
    caption = "Example Table"

    bindEach(demoList) {
        div {
            div {
                p {
                    span(it)
                }
            }
        }
    }
}

Here, we actually get the expected result, i.e. ENTRY 2 and ENTRY 3 are removed from the DOM: grafik

Did I miss something out on in the documentation or could this perhaps be a bug?

If anyone is interested, I've put all of the above into a small example project: https://github.com/simwagner/kvision-table-issue

Thanks for your awesome work! :)

rjaros commented 1 year ago

Hello and thank you for your report. This is not really a bug - it's just the way bindEach and row functions are implemented. Unlike other bind functions, which are implemented using a generic receiver (W.(T) -> Unit), the bindEach function takes a lambda parameter with non-generic signature (SimplePanel.(S) -> Unit). That's a requirement of its internal implementation. But unfortunately this makes bindEach incompatible with a few KVision DSL builder functions, which are implemented for specific receivers only (row() is one of them, bacause it's declared as fun Table.row(...)).

I've played a bit with your example and I've even found a workaround. Adding this extension function fixes the problem in this specific case. But it's probably a bit tricky to add to KVision codebase ;-)

fun Container.row(init: (Row.() -> Unit)? = null) {
    (this.unsafeCast<Table>()).row(null, init)
}

I'll keep this issue open and I'll think about it a bit more.

rjaros commented 1 year ago

I've changed bindEach implementation and factory lambda signature. The problem will be fixed in the next release.

rjaros commented 1 year ago

Fixed in 6.0.6

simwagner commented 1 year ago

That's awesome, thanks! :)