Comments (17)
Nice catch! Will investigate, thanks!
from compose-router.
Findings so far:
- There's a
backStackMap
insideRouter.kt
which holds reference to BackStack instances - Since
BackStack
is@Model
, it seems it has references to Composables under the hood (it makes sense that it does), that's the source of the leak - Removing
backStackMap
means it's lost with DKA - Making it ambient again makes no difference, leaks the same
- Instead of storing
BackStack
, an option is to only store its elements (which are then not@Model
instances) and restoreBackStack
from elements, but this has further problems:- If elements are immutable list, and list operations create new lists inside
BackStack
, then the held reference to list won't get updated, so upon restoring frombackStackMap
it will restore old state - If elements are mutable list, then
@Model
cannot track changes inside that, and will not trigger a recompose.
- If elements are immutable list, and list operations create new lists inside
Options at this point that I don't really want to do:
- Force extending either
Serializable
orParcelable
on all sealed classes that Router can accept, so that they are persistable without app-level map directly toBundle
- Drop support for persisting back stack
Optimally:
- Find a way to persist
BackStack
on app-level without references to Composables
Fallback:
- Maybe make separate packages, then client code can pick whether it wants transient / serializable / parcelable
Router
- I've had this idea earlier
from compose-router.
What do you mean by DKA?
from compose-router.
DKA = Don't Keep Activities
from compose-router.
Ah OK thanks for clarification.
It seems to me that backStackMap
is a global value, so technically there should be no need for storing anything in savedInstanceState
? Although this would not help with the leak, just trying to understand why it's necessary now
from compose-router.
Would it be possible to cache a non @Model
class inside backStackMap
and wrap it with a @Model
class before passing it down to children?
from compose-router.
Yes, now it's global. Original intention was to store it in Bundle
, but 1. it means all sealed classes need to be either Serializable
or Parcelable
2. Actual elements are lightweight objects so I saw no hurt in storing them global in a trade-off for simpler client code.
Bundle
on the other hand is used for more than that. You have the BundleScope
@Composable
which allows client code to store and retrieve data from a scoped Bundle
. This is demonstrated in the example app found in app-nested-containers
module. There you have the "Local data: 0" labels which are actually clickable to increase the counter. When you leave a routing behind (parent level pushes something to back stack) but come back to it (pop), that data is restored from Bundle
.
What you described as caching non-@Model
class is what I tried to describe above with storing only the elements of BackStack
where I wrote about why neither a mutable nor an immutable list would work unfortunately.
from compose-router.
I just noticed something in those logs. I think the issue is not with the @Model
class. Here's what happens:
- Start app -
onActive
called - Press back to quit app -
onDispose
NOT called - Start app again -
onActive
called
Seems like this way Router
gets leaked
from compose-router.
Actually if you do this you will see same issue for onDispose
not called
setContent {
onActive {
Log.d("ASD", "onActive")
}
onDispose {
Log.d("ASD", "onDispose")
}
}
from compose-router.
It is partially related to the model class, but you are right, I have checked it yesterday and noticed the same issue.
from compose-router.
There's more discussion in Slack, but the workaround for now is to store a reference to Composition
returned by setContent {}
and then call Composition.dispose()
inside onDestroy
in Activity
from compose-router.
Nice! Do you want to make a PR?
from compose-router.
Well this problem cannot really be solved in Router
and has to be solved in client code. Plus seems like Google team is aware and said it should be fixed with next release.
from compose-router.
I meant there are 2 example apps and the main README could also make a mention of it.
from compose-router.
I just thought to have your name on it since you found the solution. If you don't feel like it, I can do it too.
from compose-router.
I cannot do it just now, so you go ahead and do it :) The solution was suggested on Slack #compose ;)
from compose-router.
Closing this, because this bug is in Compose not in this library
from compose-router.
Related Issues (20)
- Back stack is not in order
- 0.1.0-dev06 - BottomNavigation Sample HOT 4
- Router crashes when using compose `0.1.0-dev08` HOT 1
- Make example apps independent of library version
- Unable to import when built with com.android.tools.build:gradle > 4.1.0-alpha05 HOT 3
- Crash with 0.11.1
- Please update to dev12 HOT 1
- Top of Stack HOT 5
- Off by one error in replace HOT 1
- [Proposal] Expose read access for BackStack#elements HOT 3
- [Question] Integration with ViewModelStore? HOT 1
- Crash with Jetpack Compose Alpha07 HOT 4
- How to handle TopAppBar navigation back event?
- Second launch icon HOT 1
- Open source license HOT 1
- Multiplatform support
- How do I save the state in a previous view? HOT 9
- Support for Dynamic Features?
- ViewModels not cleared when navigation out
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from compose-router.