Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mutable state tied directly to an URL parameter #266

Open
jakobkmar opened this issue Feb 19, 2023 · 8 comments
Open

Mutable state tied directly to an URL parameter #266

jakobkmar opened this issue Feb 19, 2023 · 8 comments

Comments

@jakobkmar
Copy link

Is there a simple way to tie some kind of mutable state directly to an URL parameter in a simple way?

val myParameter by remember { urlParameterState("myKey") }

// changing the state also manipulates the parameter in the url
myParameter = listOf("new value")

I think this is a pretty common usage of url parameters in compose for web.
Does something like that already exist in the library? And if not, what would be the best way to achieve this abstraction?

@hfhbd
Copy link
Owner

hfhbd commented Feb 19, 2023

Hey, that sounds interesting and indeed useful!

There is no built-in method to match this behavior. How exactly would you use it?

HashRouter(initPath = "/") {
  route("/todos") {
    int { todoItem: MutableState<Int> ->
      todoItem.set(42) // will update the path to "/todos/42"
  }
}

To support Parameters some kind of key mapping would be needed:

```kotlin
HashRouter(initPath = "/") {
  route("/todos") {
    var todoItem: Int by rememberParameter("todoItem")
    todoItem = 42 // will update the path to "/todos?todoItem=42"
  }
}

And if not, what would be the best way to achieve this abstraction?

Do you mean as PR or using user code?
If first: both implementations should be added to RouteBuilder and require using the local Router and its navigate(to) methods.
Regarding user code: I don't think the first api idea is possible to implement because RouteBuilder uses internal/private apis. But I think, you should be able to implement rememberParameter by yourself, but using the internal apis would be way more performant (with some refactoring using mutableStateMapOf first).
Maybe this work, I didn't test it, specially the custom mutableState!.

@Composable
public fun <T> RouteBuilder.rememberParameter(key: String): MutableState<List<T>?> {
    val router = Router.current
    return remember(key) {
        val state = mutableStateOf(parameters?.map?.get(key)?.map { it as T })
        UpdatedParameterState(state) {
            val old = parameters?.map?.toMutableMap() ?: mutableMapOf()
            if (it != null) {
                old[key] = it.map { it.toString() }
            } else {
                old.remove(key)
            }
            router.navigate(to = router.currentPath.path, parameters = old)
        }
    }
}

private class UpdatedParameterState<T>(
    private val state: MutableState<T>,
    private val update: (T) -> Unit
) : MutableState<T> by state {
    override var value: T
        get() = state.value
        set(value) {
            update(value) // Unsure, if this is recommended
            state.value = value
        }
} 

PR is always welcome, otherwise I will take it later.

@jakobkmar
Copy link
Author

Thank you for the fast reply, your method works (apart from the invalid casts).

It is similar to what I have come up with first, which was the following:

private typealias ParameterState = MutableState<List<String>?>

private class DelegateParameterState(
    val key: String,
    private val router: Router,
    private val internalState: ParameterState,
) : ParameterState by internalState {

    override var value: List<String>?
        get() = internalState.value
        set(value) {
            val params = (router.currentPath.parameters?.map ?: emptyMap()).toMutableMap()
            if (value != null) {
                params[key] = value
            } else {
                params -= key
            }
            router.navigate(router.currentPath.path, params)
        }
}

@Composable
fun rememberUrlParameterState(key: String): ParameterState {
    val router = Router.current

    val value = remember(router.currentPath) {
        router.currentPath.parameters?.map?.get(key)
    }
    return remember(key, value) {
        DelegateParameterState(key, router, mutableStateOf(value))
    }
}

The issue with both solutions seems to be, that they trigger two recompositions instead of just one. This is not what should ideally happen, right?

Regarding the PR, I would be open for implementing something like this, but not until I am sure that the implementation triggers the least amount of recompositions possible.

@hfhbd
Copy link
Owner

hfhbd commented Feb 23, 2023

Yes, this might be a problem. The first composition runs because of the mutable state, the second one during resolving the updated url, the third one during routing of the new url. I don't have a workaround/better api for the last composition, because the tree is mutable (you can enable/disable routes during runtime). But maybe, the new first composition could be skipped using some other (internal?) APIs (or implementing MutableState by ourself?).

I think a PR would be okay though, designing the API is important too, we should be able to improve the performance later.

AFAIK there are no Compose Web performance tools to check it, but maybe we could reuse the tools from androidx, there is already a jvm target 🤔

@jakobkmar
Copy link
Author

Since the double recomposition happens due to the url changing, I think it is a general issue with the router causing the whole content to recompose on any change in the url, and not just if the relevant part changed.

e.g.:

BrowserRouter(initPath = "/") {
    route("/") { HomePage() }
    route("/other") { OtherPage }
}

The body of the /other route should only be called again / recomposed if that part of the url changed - and not any part lower down the chain.

Should I open a seperate issue for this?

@hfhbd
Copy link
Owner

hfhbd commented Mar 12, 2023

The body of the "/other" route should only be called again / recomposed if that part of the url changed - and not any part lower down the chain.

This isn't the case at the moment?

BrowserRouter(initPath = "/") { // 1
    route("/") { // 2
      HomePage() 
    }
    route("/other") { // 3
     OtherPage
    }
}

After an URL change or the init run, the content of the composable builder (1) runs.
Lets talk about the init run with "/". If there is a route (it is "/"), its content is called, here 2.
After this leaf (route("/")), an internal state is set to notify the builder: "I got a match, don't try another routes". In fact, this emulates a when but there is no functional/dsl when supporing live updates in Kotlin.
So updating the internal match state causes the builder to run again (1). This cause another run of (2) too, ideally, this could be skipped, because there shouldn't be any changes (edit: it is skipped using remember(path))
But there shouldn't be a run of "/other"

If so, please open another issue.

@jakobkmar
Copy link
Author

jakobkmar commented Mar 12, 2023

Having done some further checks, seems like the above stated by me isn't the case, but somehow both our solutions cause things to recompose which shouldn't be updated at all. So not just too many recompositions, but also at an unnecessary scale.

For example, using the mutable url paramter state in an attributes block causes the whole composable and all of its children to recompose, while with regular mutable state only the attributes themselves get recomposed.

@hfhbd
Copy link
Owner

hfhbd commented Mar 12, 2023

Do you have a code snippet?
Anyway, I won't take a look before next week.

@jakobkmar
Copy link
Author

I have experimented with different configurations, the current one is this test page:
https://gist.github.com/jakobkmar/5487d94180966c972017f127f0d88170

You can also replace the state implementation below with yours, it'll have the same result (at least when it comes to the println messages).

To see the behaviour with regular state (but still calling navigate), just uncomment the two lines in this snippet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants