Add opportunity to use multiple refs in attributes

[InsanusMokrassar]

I am coding a lot of things in compose for html and from time to time I facing with the problem that there is no opportunity to make configuration of ref in attributes from different places which should not know about each other. By this PR I am proposing solution of this problem

This should be tested by QA

Release Notes

Add opportunity to use several refs in one attribute section

[InsanusMokrassar]

Hello :) is there some comments/recommendations on this PR?

[InsanusMokrassar]

Could you please describe a bit more the use cases when it's needed?

For example, if I am making several independent extensions for an Elements and this PR would give an opportunity to add extensions for AttrScope and do whatever these extensions require there, including work with ref ans onDispose. In most cases it will be better if these extension will handle by themself everything they need

[InsanusMokrassar]

@eymar could you please review this PR again?

[eymar]

several independent extensions for an Element

Could you please show some code example, even hypothetical? How those extensions are added/applied to an element.

I'm wondering if your extensions could be collected, then merged into 1 under the hood, so they would require only 1 refEffect.

[InsanusMokrassar]

several independent extensions for an Element

Could you please show some code example, even hypothetical? How those extensions are added/applied to an element.

I'm wondering if your extensions could be collected, then merged into 1 under the hood, so they would require only 1 refEffect.

Got it, will write today here, in comments

[InsanusMokrassar]

several independent extensions for an Element

Could you please show some code example, even hypothetical? How those extensions are added/applied to an element.

I'm wondering if your extensions could be collected, then merged into 1 under the hood, so they would require only 1 refEffect.

Lets imagine we have two tasks: listen clicking outside of elements and listen for hovers on elements with some classnames.

First solution:

fun HtmlElement.checkParent(lookingForParent: Element) = this == lookingForParent || (parent ?.checkParent(lookingForParent) == true)

fun AttrScopeBuilder<*>.onClickOutside(block: () -> Unit) {
    ref { element ->
        val listener = {
             if (!it.target.checkParent(element)) block()
        }
        document.addEventListener("click", listener)
        onDispose {
            document.removeEventListener("click", listener)
        }
    }
}

Second one:

fun AttrScopeBuilder<*>.onOtherElementHover(classname: String, classnameOnHoverIn: String) {
    val mutationObserver = // init of mutation observer for listening elements with classname
    ref {
        // including classnameHoverIn to it.classList when some of elements with classnamr hovered in and removing otherwise
        onDispose {
            mutationObserver.disconnect()
            // removing of additional listeners
        }
    }
}

The last sample is not full, because good solution would be to huge, but idea is that than you may use it:

Div ({
    onClickOutside {}
    onOtherElementHover(...)
})

And each extension utilize the things it need in order it need

[eymar]

I'd like to discuss an alternative, which doesn't require the implementation changes:

Div(attrs = {
    //..
}) {
    DisposableEffect(1) {
       val listener = {...}
        scopeElement.addEventListener("click", listener)
        onDispose { 
            // remove the listener
        }
    }
    DisposableEffect(2) {
       val listener = {...}
        scopeElement.addEventListener("mouseover", listener)
        onDispose { 
            // remove the listener
        }
    }
}

scopeElement - is an extension property available in the scope of DisposableEffect.

So you can add as many extensions you like in Div content (not in the attrs scope though). The functions would need to be @Composable in order to call DisposableEffect. If I understood your idea correctly, it would give the same results.

Would it work for you?

[InsanusMokrassar]

It could be the solution, but:

DisposableEffect(0) {
    onDispose {}
}

Is a little bit complicated in comparison with:

ref {
    onDispose {}
}

Besides, it is not direct usage of DisposableEffect, because it has been created to track some value (as I understand) which means that it will be more complex in comparison with ref. In my opinion, it is not kotlin way :)

[InsanusMokrassar]

And, imho, if compare snippets:

Div({
    onClickOutside {}
    onHover... {}
}) {}

And:

Div({}) {
    OnClickOutside {} // it is NOT component, but used in elements scope
    OnHover {} // same
}

The last one is less... Obvious and usefull

[InsanusMokrassar]

@eymar hi, could you please review this PR?

[eymar]

Besides, it is not direct usage of DisposableEffect, because it has been created to track some value (as I understand) which means that it will be more complex in comparison with ref

ref uses DisposableEffect under the hood too. They key passed into it can be just Unit.

I see a nuance with conditional extensions:

ref {
    if (useDefaultOnHover) onHover() else onCustomHover()
}

It won't work as we would expect when the condition changes. The ref would be applied only during the first composition. If the condition changes later, ref won't be reapplied. Perhaps, it's a bug which hasn't affected anyone yet.

But since this API lets developer write the code that doesn't work in some cases, I think using an alternative would be better. I would like to not build new features on top of the problematic API (ref in this case).

The alternative way with Composable extensions would work correctly with conditions.

[InsanusMokrassar]

Besides, it is not direct usage of DisposableEffect, because it has been created to track some value (as I understand) which means that it will be more complex in comparison with ref

ref uses DisposableEffect under the hood too. They key passed into it can be just Unit.

I see a nuance with conditional extensions:

ref {
    if (useDefaultOnHover) onHover() else onCustomHover()
}

It won't work as we would expect when the condition changes. The ref would be applied only during the first composition. If the condition changes later, ref won't be reapplied. Perhaps, it's a bug which hasn't affected anyone yet.

But since this API lets developer write the code that doesn't work in some cases, I think using an alternative would be better. I would like to not build new features on top of the problematic API (ref in this case).

The alternative way with Composable extensions would work correctly with conditions.

You are right, but in my examples there are no any conditions :) Besides, changes of this PR do not change meaning of ref behaviour - it is still one time callback with on dispose. If to talk about conditions, you sample above:

DisposableEffect(0) {
    if (some) doSome() else doOther()
    onDispose {}
}

Will not follow some changes as well as ref. Do you have some remarks or commentaries on code? Is there any reasons to prevent this PR merging?

[eymar]

DisposableEffect(0) {
    if (some) doSome() else doOther()
    onDispose {}
}

This won't work - right. But if you have extensions, you can have:

@Composable 
fun Foo() {
   Div {
       if (useDefaultOnHover) ExtDefaultHover() else ExtCustomHover()
   }
}

Is there any reasons to prevent this PR merging?

While it mostly preserves the existing behaviour, it adds a new behavior on top of the existing one. A problem it tries to solve (a need for a possibility to add/remove/manage extension on the html elements) is clear to me. It's possible to achieve that using an alternative (maybe less conveniently).

We tend to be careful with adding new features/behaviours since it's much harder to remove them later if needed (an option is only to deprecate it). In this case I have doubts, since the ref {} is problematic itself, and it might be a candidate for deprecation and further improvements. I see that this area/topic requires more deliberation. Perhaps, we'll have more resources to dig into it in the future.

Your PR and feature request revealed an issue with the existing API - ref. I created a ticket for us to follow up: https://youtrack.jetbrains.com/issue/CMP-5980

[InsanusMokrassar]

DisposableEffect(0) {
    if (some) doSome() else doOther()
    onDispose {}
}

This won't work - right. But if you have extensions, you can have:

@Composable 
fun Foo() {
   Div {
       if (useDefaultOnHover) ExtDefaultHover() else ExtCustomHover()
   }
}

Is there any reasons to prevent this PR merging?

While it mostly preserves the existing behaviour, it adds a new behavior on top of the existing one. A problem it tries to solve (a need for a possibility to add/remove/manage extension on the html elements) is clear to me. It's possible to achieve that using an alternative (maybe less conveniently).

We tend to be careful with adding new features/behaviours since it's much harder to remove them later if needed (an option is only to deprecate it). In this case I have doubts, since the ref {} is problematic itself, and it might be a candidate for deprecation and further improvements. I see that this area/topic requires more deliberation. Perhaps, we'll have more resources to dig into it in the future.

Your PR and feature request revealed an issue with the existing API - ref. I created a ticket for us to follow up: https://youtrack.jetbrains.com/issue/CMP-5980

Now ref is well documented and well-obvious excluding the momemnt with the exclusive requirement to use it once. Currently ref is not deprecated and for me it means that this feature is stable and extendable by PRs like this. In case you think this feature (ref) is redundant and must be removed in future - it would be nice to describe how can I understand in future which features supposed to be bad ones.

About the if clauses with states of changeable values: it is documented that they will be tracked only in composable contexts. If you think that ref is a bad feature due to its inobvious, all attribute blocks must be removed too due to their non composable nature.

[InsanusMokrassar]

@eymar I have created the issue for this PR to make it easier to find it in issues of compose multiplatform

[eymar]

In case you think this feature (ref) is redundant and must be removed in future - it would be nice to describe how can I understand in future which features supposed to be bad ones.

The problems in API design are not always obvious and easy to spot. And I think there is no guaranteed way to know in advance. In this case, it's revealed that a valid kotlin code (with if..else..) doesn't work with ref at runtime as it's expected. We learnt it only now.

all attribute blocks must be removed too due to their non composable nature.

I'd say it's not about "composable or not" nature of the block. attrs block is re-applied on all composition/recompositions, unlike ref.

[InsanusMokrassar]

In case you think this feature (ref) is redundant and must be removed in future - it would be nice to describe how can I understand in future which features supposed to be bad ones.

The problems in API design are not always obvious and easy to spot. And I think there is no guaranteed way to know in advance. In this case, it's revealed that a valid kotlin code (with if..else..) doesn't work with ref at runtime as it's expected. We learnt it only now.

all attribute blocks must be removed too due to their non composable nature.

I'd say it's not about "composable or not" nature of the block. attrs block is re-applied on all composition/recompositions, unlike ref.

Anyway, I would like to understand, should I expect some notes about this PR or it will be merged whenever or closed?

[eymar]

I apologise for a delay. Currently we don't have capacity to approach the ref API design and implementation issues. This PR is noted in https://youtrack.jetbrains.com/issue/CMP-5980 and it won't be lost anyway. Perhaps, it's better to close it so it's not assumed that it's being actively worked on.

Thank you for your efforts and contributions!

[InsanusMokrassar]

~~Maybe it would be better to extract compose-html as some external library in case if multiplatform team now is not aimed to support html? It looks like it would be better to allocate somebody in JB to work with compose html only to make at least some support for library? Cause now it looks like JB trying to bury compose html in favor to compose wasm when compose html works better if compare to compose wasm.~~

Ok, thank you for your answer :) I will close this PR