Interoperability and wasmJs bugfixes#412
Conversation
…aks and performance degradation after large number of recompositions
| val path = Path.from(rawPath) | ||
| val node = remember(path) { RouteBuilder(path.path, path) } | ||
| val node = remember(path) { | ||
| RouteBuilder.routeBuilderCache.getOrPut("${this.hashCode()} ${path.path} $path") { |
There was a problem hiding this comment.
Why can't we put all parameters to remember? Remember uses its own cache.
There was a problem hiding this comment.
Remember only works with a recomposition. The routing changes the composition (route calls are leaving and entering the composition when navigating) and remember doesn't work here.
There was a problem hiding this comment.
Do you have any samples causing the memory leaks (or a reproducer)? I do understand the cause, we create a new RouteBuilder during each composition, but ideally the GC/Compose should be able to remove the object.
There was a problem hiding this comment.
I don't have a simple reproducer because this effect is only visible after thousands of recompositions and I've measured this only when running my app as a SSR service. The problem is not about RouteBuilder itself but about Compose MutableState it contains. I think there are subscriptions created for each instance by the compose compiler and I think they are not disposed correctly.
| routeBuilder: @Composable RouteBuilder.() -> Unit | ||
| ) { | ||
| BrowserRouter().route(initPath, routeBuilder) | ||
| Router.internalGlobalRouter = BrowserRouter().apply { route(initPath, routeBuilder) } |
There was a problem hiding this comment.
What's the use-case/code for setting this variable? Can't the function just return the Router instead?
There was a problem hiding this comment.
It could. I've implemented it this way just to simplify the client code and to be consistent with Router.current. In my applications I just use Router.global when Router.current can't be called.
There was a problem hiding this comment.
What do you mean with can't be called? Because Router.current is @Composable or because there is no value set?
There was a problem hiding this comment.
It can't be called from any non-composable functions like effects, event handlers, view model methods or some global utility functions.
|
BTW I will cherry pick your wasm fixes in another PR. |
3 participants
This PR fixes wasmJs navigation as well as some memory leaks. It also makes the library more open and easier to extend (I'm using it in my Kilua framework).