mvysny / vaadin-on-kotlin

Writing full-stack statically-typed web apps on JVM at its simplest
https://www.vaadinonkotlin.eu/
MIT License
186 stars 17 forks source link

Update to support new DataProvider enhancements in Flow 4.0 #51

Closed jhult closed 4 years ago

jhult commented 4 years ago

There are updates needed to support the new DataProvider improvements that are being done in Vaadin #8054.

Example:

java.lang.IllegalStateException: Grid uses Vaadin built-in ListDataProvider however the filter is VoK's Filter, which is incompatible with Vaadin's ListDataProvider. Please use `grid.setDataLoaderItems()` to use the DataLoader API instead (which is much simpler anyway).
    at eu.vaadinonkotlin.vaadin10.FilterBar.applyFilterToGrid(FilterBar.kt:116) ~[vok-util-vaadin10-master-22926d5677-1.jar:?]
    at eu.vaadinonkotlin.vaadin10.FilterBar.updateFilter(FilterBar.kt:169) ~[vok-util-vaadin10-master-22926d5677-1.jar:?]
    at eu.vaadinonkotlin.vaadin10.FilterBar.access$updateFilter(FilterBar.kt:91) ~[vok-util-vaadin10-master-22926d5677-1.jar:?]
    at eu.vaadinonkotlin.vaadin10.FilterBar$finalizeBinding$reg$1.invoke(FilterBar.kt:348) ~[vok-util-vaadin10-master-22926d5677-1.jar:?]
    at eu.vaadinonkotlin.vaadin10.FilterBar$finalizeBinding$reg$1.invoke(FilterBar.kt:91) ~[vok-util-vaadin10-master-22926d5677-1.jar:?]
    at eu.vaadinonkotlin.vaadin10.FilterBar$Binding$addFilterChangeListener$1.valueChanged(FilterBar.kt:222) ~[vok-util-vaadin10-master-22926d5677-1.jar:?]
    at com.vaadin.flow.component.internal.AbstractFieldSupport.lambda$addValueChangeListener$828eca10$1(AbstractFieldSupport.java:96) ~[flow-server-4.0-SNAPSHOT.jar:4.0-SNAPSHOT]
    at com.vaadin.flow.component.internal.AbstractFieldSupport$$Lambda$874/0000000000000000.onComponentEvent(Unknown Source) ~[?:?]
    at com.vaadin.flow.component.ComponentEventBus.fireEventForListener(ComponentEventBus.java:205) ~[flow-server-4.0-SNAPSHOT.jar:4.0-SNAPSHOT]
    at com.vaadin.flow.component.ComponentEventBus.fireEvent(ComponentEventBus.java:194) ~[flow-server-4.0-SNAPSHOT.jar:4.0-SNAPSHOT]
    at com.vaadin.flow.component.Component.fireEvent(Component.java:378) ~[flow-server-4.0-SNAPSHOT.jar:4.0-SNAPSHOT]
    at com.vaadin.flow.component.ComponentUtil.fireEvent(ComponentUtil.java:385) ~[flow-server-4.0-SNAPSHOT.jar:4.0-SNAPSHOT]
    at com.vaadin.flow.component.internal.AbstractFieldSupport.setValue(AbstractFieldSupport.java:207) ~[flow-server-4.0-SNAPSHOT.jar:4.0-SNAPSHOT]
    at com.vaadin.flow.component.internal.AbstractFieldSupport.setModelValue(AbstractFieldSupport.java:167) ~[flow-server-4.0-SNAPSHOT.jar:4.0-SNAPSHOT]
    at com.vaadin.flow.component.AbstractField.setModelValue(AbstractField.java:225) ~[flow-server-4.0-SNAPSHOT.jar:4.0-SNAPSHOT]
    at com.vaadin.flow.component.AbstractSinglePropertyField.handlePropertyChange(AbstractSinglePropertyField.java:352) ~[flow-server-4.0-SNAPSHOT.jar:4.0-SNAPSHOT]
    at com.vaadin.flow.component.AbstractSinglePropertyField.access$200(AbstractSinglePropertyField.java:48) ~[flow-server-4.0-SNAPSHOT.jar:4.0-SNAPSHOT]
    at com.vaadin.flow.component.AbstractSinglePropertyField$1.propertyChange(AbstractSinglePropertyField.java:325) ~[flow-server-4.0-SNAPSHOT.jar:4.0-SNAPSHOT]
    at com.vaadin.flow.internal.nodefeature.ElementPropertyMap.lambda$fireEvent$2(ElementPropertyMap.java:462) ~[flow-server-4.0-SNAPSHOT.jar:4.0-SNAPSHOT]
    at com.vaadin.flow.internal.nodefeature.ElementPropertyMap$$Lambda$812/0000000000000000.accept(Unknown Source) ~[?:?]
    at java.util.ArrayList.forEach(ArrayList.java:1507) ~[?:?]
    at com.vaadin.flow.internal.nodefeature.ElementPropertyMap.fireEvent(ElementPropertyMap.java:462) ~[flow-server-4.0-SNAPSHOT.jar:4.0-SNAPSHOT]
    at com.vaadin.flow.internal.nodefeature.ElementPropertyMap.access$100(ElementPropertyMap.java:48) ~[flow-server-4.0-SNAPSHOT.jar:4.0-SNAPSHOT]
    at com.vaadin.flow.internal.nodefeature.ElementPropertyMap$PutResult.run(ElementPropertyMap.java:169) ~[flow-server-4.0-SNAPSHOT.jar:4.0-SNAPSHOT]
    at com.vaadin.flow.server.communication.ServerRpcHandler.runMapSyncTask(ServerRpcHandler.java:395) [flow-server-4.0-SNAPSHOT.jar:4.0-SNAPSHOT]
    at com.vaadin.flow.server.communication.ServerRpcHandler.lambda$handleInvocations$0(ServerRpcHandler.java:389) [flow-server-4.0-SNAPSHOT.jar:4.0-SNAPSHOT]
    at com.vaadin.flow.server.communication.ServerRpcHandler$$Lambda$776/0000000000000000.accept(Unknown Source) [flow-server-4.0-SNAPSHOT.jar:4.0-SNAPSHOT]
    at java.util.ArrayList.forEach(ArrayList.java:1507) [?:?]
jhult commented 4 years ago

I think the above exception is okay. I inadvertently caused this by switching away from a DataLoader and using setItems (with my List) directly on a Grid. I did this because of the real issue: Grid no longer implements HasDataProvider. I believe fix is to update eu.vaadinonkotlin.vaadin10.vokdb.DataProviders similar to the following:

import com.github.mvysny.vokdataloader.DataLoader
import com.github.vokorm.KEntity
import com.vaadin.flow.data.provider.DataView
import com.vaadin.flow.data.provider.HasDataView
import eu.vaadinonkotlin.vaadin10.DataLoaderAdapter
import eu.vaadinonkotlin.vaadin10.VokDataProvider

/**
 * Sets given [dataLoader] to this [Grid], by the means of wrapping the [dataLoader] via [DataLoaderAdapter] and setting it
 * as a (configurable) [com.vaadin.flow.component.grid.Grid.Grid.getDataProvider].
 */
fun <T: KEntity<*>, V: DataView<T>> HasDataView<T, V>.setDataLoader(dataLoader: DataLoader<T>) {
    setItems(dataLoader.asDataProvider())
}

/**
 * Returns a [VokDataProvider] which loads data from this [DataLoader].
 */
fun <T: KEntity<*>> DataLoader<T>.asDataProvider(): VokDataProvider<T> = DataLoaderAdapter(this) { it.id!! }

eu.vaadinonkotlin.vaadin10.DataLoaderAdapter.DataLoaderAdapter needs some updates as well:

import com.vaadin.flow.data.provider.HasDataView
/**
 * Sets given [dataLoader] to this [Grid], by the means of wrapping the [dataLoader] via [DataLoaderAdapter] and setting it
 * as a (configurable) [Grid.getDataProvider].
 * @param idResolver provides unique ID for every item. The ID is then used to differentiate items.
 * See [com.vaadin.flow.data.provider.DataProvider.getId] for more details. Typically every item
 * has a primary key of type [Long], but any Java/Kotlin object with properly written [Any.equals] and [Any.hashCode] can act as the ID,
 * including the item itself.
 */
fun <T: Any, V: DataView<T>> HasDataView<T, V>.setDataLoader(dataLoader: DataLoader<T>, idResolver: (T)->Any) {
    setItems(dataLoader.asDataProvider(idResolver))
}
mvysny commented 4 years ago

Sounds good, thanks! I'm currently on a vacation but I'll take a look next month :+1:

mvysny commented 4 years ago

Thank you for letting me know :+1: It feels a bit weird that Grid would simply stop implementing HasDataProvider since that's a backward-incompatible change. However, if that's the case, then we need to figure out how to add extension methods both to HasDataProvider and to HasDataView while keeping some kind of compatibility with Vaadin 14...

@jhult Which Vaadin version are you testing with please?

jhult commented 4 years ago

I'm using a snapshot of Vaadin 17 with Flow 4.

Relevant discussion can be found here.

mvysny commented 4 years ago

You're right, in Vaadin 17 the Grid will no longer implement HasDataProvider. One solution would be to compile against Vaadin 17 and add the code as you suggest, however Vaadin Gradle Plugin unfortunately doesn't support Vaadin 17 and so the example project would stop working...

Alternatively, since Grid will not lose the setDataProvider() function, we could introduce an overload specifically for Grid:

fun <T: KEntity<*>> Grid<T>.setDataLoader(dataLoader: DataLoader<T>) {
    setItems(dataLoader.asDataProvider())
}

That way VoK can still be compiled with Vaadin 14 but would still work with Vaadin 17. This could be a temporary solution until next Vaadin LTS is released, then we can completely move to HasDataView and drop the HasDataProvider interface.

I need to check though whether the autocompletion will not be too confusing (offering two setDataLoader() functions for Grid).

mvysny commented 4 years ago

The binary compatibility is now out of the question because of https://github.com/vaadin/flow/issues/8831 . Now we have two options:

  1. Branch vok and maintain two separate branches, one for Vaadin 17+, other for Vaadin 16-
  2. Compile VoK with Vaadin 17 and hope that HasDataProvider.setDataLoader() extension methods will still active properly under Vaadin 14.

I'd like to avoid 1 if possible, because it introduces a constant need for merging stuff between the two branches. I had to go with 1. with Karibu-Testing and it's a bit annoying.

jhult commented 4 years ago

I agree option 1 sounds like a pain and should be avoided if possible. Let me know if I can help in any way.

mvysny commented 4 years ago

Perhaps the best way could be having an additional module for Vaadin 17+... I'll think about this option.

mvysny commented 4 years ago

The new functionality is now present in new module vok-framework-v17-vokdb.