Hi @rkishan516 do you still have plans for this change?

Hi @Piinks, This was a proposal PR for @justinmc to review. If we want to continue with this, then I can surely update this.

This has come back around to stale PR triage. We don't review draft PRs, but have you reached out on Discord for the feedback you were seeking?

This has come back around to stale PR triage. We don't review draft PRs, but have you reached out on Discord for the feedback you were seeking?

Hey, last time we couldn't conclude, I will continue that discussion again today.

It looks like there are two failing customer tests in the provider package, and they're kind of concerning. I wouldn't expect this change to cause them to fail, but maybe it's because I don't understand the inner workings of provider. Do you know what's causing the failure?

https://github.com/rrousselGit/provider/blob/558bdcd26ddb2b922bec06e3fa0d91bc59479baf/packages/provider/test/null_safe/context_test.dart#L489-L520

The test is incorrect as per Providers own documentation: https://pub.dev/documentation/provider/latest/provider/ChangeNotifierProvider-class.html#:~:text=DON%27T%20reuse%20an%20existing%20ChangeNotifier%20using%20the%20default%20constructor

I think I agree with @rrousselGit in the original issue (#106549), who said that this should probably be opt-in. I really can't think of any reasonable situation where you would logically need the original behavior, can you? But I worry due to the (slight) performance effect.

Overall I agree that Flutter should have this feature, probably behind an opt-in flag.

Maybe tests could be run if this actually affects performance? Because I can also come up with some pathological cases where this should boost performance.

IMO it would be too bad to have the desired / correct behaviour only available behind a flag / different API & having to wait for the entire ecosystem to migrate.

Thanks @justinmc! I've investigated the failing provider tests and found the bug in my implementation.

The issue was in where cleanupRemovedDependencies() was being called. My original implementation called it only once on the root context element in buildScope(), but _flushDirtyElements rebuilds many elements during a build cycle.

For the fix, I moved cleanupRemovedDependencies from BuildOwner.buildScope() to ComponentElement.performRebuild. Now each element cleans up its own dependencies after it rebuilds.

On the opt in choice, I am not really sure. I can see some advantage of opt in, so for now I am also leaning towards it.

Since the current behavior (never unsubscribe) is intentional, I assume we'll never make this behavior the default. In this case, do you think it'll be easier to have another method to call than to configure this when defining the entire class? This allows the app developers to decide whether to adopt it, so that this feature can be applied to existing inherited models as well.

I'm imagining a removeDependency method, since the developer should know the timing that they're no longer using the dependency. Or we can have a slightly smarter method such as checkDependencyCleanup. In general my idea is that if the developer feels like unsubscribing, they can call it in similar places as the dependOn methods.

The very reason this feature is requested is that developers dont have a good place to unsubscribe.

Folks can already override InheritedElement.removeDependent.
They thing is that the only mean for a package to know when a dependency is no-longer useful is to override ComponentElement.build. That requires dissociating from Stateless/StatefulWidget, making this solution incompatible with a large chunk of the ecocystem.

(the alternative of providing Builder-like widgets has the same compatibility issue)

The implementation most looks good to me, just left some suggestion to see if we can squeeze out as much performance as possible

@rkishan516 Did you try this out on a low end device? I'm interested to see the performance impact.

I have tried on iPhone 13 but i don't thats the low end, we are looking for. I will search for some old android phone and check.

Hello, thank you for the contribution and all the excellent work. While I am supportive of allowing widgets to clean up unused dependencies, I'm concerned with the current design and I don't think we should move forward with this implementation as-is:

1. A widget can depend on an inherited widget in its didChangeDependencies method. With this change, such a widget would be broken if its build method does not also depend on that inherited widget - the next rebuild might not call didChangeDependencies, which would remove the dependency.

   Example app...

   import 'package:flutter/material.dart';
   
   void main() {
     runApp(MaterialApp(home: MyPage()));
   }
   
   class MyPage extends StatefulWidget {
     @override
     State<MyPage> createState() => _MyPageState();
   }
   
   class _MyPageState extends State<MyPage> {
     int counter = 0;
    
     @override
     Widget build(BuildContext context) {
       return Scaffold(
         // InheritedA cleans up dependents.
         // InheritedB does not clean up dependents.
         body: InheritedA(
           value: counter,
           child: InheritedB(
             value: counter,
             child: Column(children: [
               InheritedText<InheritedA>(),
               InheritedText<InheritedB>(),
             ]),
           )
         ),
         floatingActionButton: FloatingActionButton(
           onPressed: () => setState(() => counter++),
           child: const Icon(Icons.add),
         ),
       );
     }
   }
   
   class InheritedText<T extends InheritedWidget> extends StatefulWidget {
     @override
     State<InheritedText> createState() => _InheritedTextState<T>();
   }
   
   class _InheritedTextState<T extends InheritedWidget> extends State<InheritedText<T>> {
     late int value;
     
     @override
     void didChangeDependencies() {
       // Use dependencies in didChangeDependencies but not build.
       final inheritedWidget = context.dependOnInheritedWidgetOfExactType<T>();
       value = switch (inheritedWidget) {
         InheritedA(value: final value) => value,
         InheritedB(value: final value) => value,
         _ => throw new Exception('Not reachable ${inheritedWidget.runtimeType}'),
       };
       super.didChangeDependencies();
     }
   
     @override
     Widget build(BuildContext context) {
       // If this widget rebuilds but the dependencies didn't change,
       // nothing uses the dependencies.
       // As a result, InheritedA will be cleaned up unexpectedly.
       return Text('Value: $value');
     }
   }
   
   class InheritedA extends InheritedWidget {
     const InheritedA({super.key, this.value = 0, required super.child});
   
     final int value;
   
     @override
     bool get cleanupUnusedDependents => true;
   
     @override
     bool updateShouldNotify(InheritedA oldWidget) => value != oldWidget.value;
   }
   
   class InheritedB extends InheritedWidget {
     const InheritedB({super.key, this.value = 0, required super.child});
   
     final int value;
   
     @override
     bool get cleanupUnusedDependents => false;
   
     @override
     bool updateShouldNotify(InheritedB oldWidget) => value != oldWidget.value;
   }

2. You can't tell whether an inherited widget will clean up unused dependents or not. For example:

   void build(BuildContext context) {
     return TextButton(
       onPressed: () {
         Foo.of(context);       
       },
       child: Text('Hello'),
     );
   }

   If I press the button, will my widget rebuild whenever Foo changes? There's no way to tell without checking Foo's implementation.

   Keep in mind that some .of methods are seriously non-trivial. For example, see Theme.of, which is actually multiple inherited widgets.

3. Do we plan to update the framework's inherited widgets to turn on cleanupUnusedDependents?

   If yes, what's that migration look like?

   If no, I don't think we should move forward with this pull request. The ideal solution would be usable by the framework.

I see that @goderbauer had expressed similar reservations on a previous pull request: #107112 (comment), #107112 (comment)

I apologize for bringing this feedback late, months after you started this pull request. Thank you again for all the excellent work!

Hello, thank you for the contribution and all the excellent work. While I am supportive of allowing widgets to clean up unused dependencies, I'm concerned with the current design and I don't think we should move forward with this implementation as-is:

Hey @loic-sharma, thanks for your feedback. It's not require to land this PR in current state or current way, our main goal is to have cleanup for unused dependency. So, if you have a better idea, i can work on that. Current implementation is just result of initial implementation + feedback based iteration.

For your concerns

1. Yes, any dependency established in didChangeDependencies, drops immediately in build method.
2. Yes, Foo decides it will cleanup dependents or not (dependents are totally unaware).
3. For framework's inherited widget, we have not thought about yet. But eventually, we might update framework's widgets.

This is a very hard problem.

What do you think of this idea instead? #106549 (comment)

I wonder if we can partially solve this problem by using the following inherited widget pattern:

1. Foo.of(context) - This looks up Foo and adds a dependency on it for the lifetime of context.
2. Foo.peek(context) - This looks up Foo but does not add a dependency on it. You can use this in your callbacks.

We'd need to add .peek(context) and .maybePeek(context) methods to all existing inherited widgets, like MediaQuery, Theme, AppLocalizations, etc...

This an easier path that would make this problem less bad. We'd still need the team to buy-off on the approach though.

This is a very hard problem.

Agree. People just ignore issue like #163467 but if we solve this, it will give clarity.

What do you think of this idea instead? #106549 (comment)

I think we already have this with dependOnInheritedWidgetOfExactType as Foo.of and getInheritedWidgetOfExactType as Foo.peek. So, its upto developer if they are exposing peek or not.

We'd still need the team to buy-off on the approach though.

Yup.