docs: clarify HomeViewModel lifecycle in Compass sample#2838
docs: clarify HomeViewModel lifecycle in Compass sample#2838harshyadavDeveloper wants to merge 2 commits into
Conversation
|
If I am not mistaken flutter style guide discourages the use of "Note:" in comments. Also, even though the comment is nice, but it just proves that in the long run using this sample as template we would need to dispose of it, comment basically discouraging people to use this sample as sample, when in reality what developers are looking for is a sample that they can safely follow, preferably without introducing third party library. This comment kind of saying that flutter doesn't provide any out of the box state management, and if it doesn't that sample is a bad sample. |
|
Thank you for the feedback! Just to make sure I'm heading in the right direction — are you suggesting that instead of adding a comment, the better fix would be to refactor the code so that HomeViewModel is created and disposed within a StatefulWidget, removing its creation from the router entirely? Wanting to confirm before I update the PR. Happy to make that change if so! |
|
@harshyadavDeveloper , at the very least, I think the Technically, the added comment already addresses the original issue (#2788) because it clarifies lifecycle ownership and explains why the current implementation is safe. That said, I still think the underlying concern remains: samples are often used as production templates, and this particular pattern becomes unsafe as soon as the ViewModel starts owning disposable resources (streams, subscriptions, controllers, etc.). The comment itself implicitly acknowledges that limitation by explaining that “real applications” should manage disposal differently. Because of that, I think there is value in considering a small refactor that demonstrates lifecycle management directly within the sample itself, ideally using Flutter’s built-in primitives rather than relying on third-party state management libraries. For example, creating and disposing the ViewModel from a StatefulWidget would make the ownership explicit and demonstrate a pattern that developers could safely copy into real applications without accidentally introducing leaks later. If the Flutter team intentionally prefers the current architecture for educational simplicity, then the explanatory comment still improves the situation significantly. |
|
Thanks for the detailed clarification — that makes sense. Instead of keeping the explanatory comment, I’m thinking of updating the sample to move That would allow the sample itself to demonstrate a lifecycle-safe pattern using Flutter’s built-in primitives, while avoiding the need for additional comments about disposal ownership. Does that direction align with what you had in mind before I proceed with the refactor? |
|
@harshyadavDeveloper, your direction sounds much closer to what I had in mind, yes. I think if such a refactor were accepted by the Flutter team, it would make the sample significantly stronger as a real-world reference implementation, because developers frequently copy architecture patterns from official samples directly into production code. Demonstrating explicit ViewModel ownership and disposal using only Flutter’s built-in primitives would:
At the same time, it would still keep the sample framework-agnostic and avoid coupling the architecture example to a particular state-management package. Of course, the Flutter team may still prefer the current approach for simplicity or separation-of-concerns reasons, but from an educational/sample perspective I think the refactor would provide long-term value if approved. |
|
Updated the sample to move This removes the need for the explanatory comment while demonstrating an explicit lifecycle-safe pattern using Flutter’s built-in primitives. |
Adds a comment explaining why
HomeViewModelis not disposed in the Compass sample.This clarifies that the current implementation is safe because the ViewModel does not manage disposable resources. It also highlights that in real applications, proper lifecycle management is required when working with streams, controllers, or subscriptions.
Fixes #2788
Pre-launch Checklist
///).