jwstegemann / fritz2

Easily build reactive web-apps in Kotlin based on flows and coroutines.
https://www.fritz2.dev
MIT License
653 stars 28 forks source link

Improve `export`-API so it cannot throw anymore #878

Closed Lysander closed 1 month ago

Lysander commented 1 month ago

Problem

Currently the export function is not safe to use:

class Exporter<T : Any>(initialize: Exporter<T>.() -> Unit) {
    lateinit var payload: T // this is bad and dangerous!

    fun export(payload: T): T {
        this.payload = payload
        return payload
    }

    init {
        initialize()
    }
}

As you can see there is the lateinit va payload: T, which gets only set, if export()-function gets calles in the initialize-Lambda.

This cannot be forced! And that leads to a brittle API :-(

Imagine some control structures like if, which prohibits the call if some value fails its condition. Then the outer export call will try to access the payload-field in order to return it. But it is still not initialized:

val something= false

export {
    if(something) {
        export("This wont get calles always!")
    }
}.also { /* an exception would be thrown before this block. Hard to find the precise code section on top! */ }

Proposed Solutions

We could add some required default: T parameter to the export()-function. This is API breaking of course, but I do not see any useful alternative, because:

The only other possibility would be to make the payloadnullable - but that would make a mockery of the whole approach! It would be much easier to declare a local nullable var and always return that instead.

As the intention is to provide an alyways returning a proper value and makes further working with that easier, we should prefer the default: T apporach.

Lysander commented 1 month ago

Final solution was to delete this function entirely!

see #879