For reference, this would later be used by Riverpod as followed (quick code just for demonstration):

import 'package:flutter/widgets.dart';

import 'internals.dart';

class ProviderContext extends InheritedWidget {
  const ProviderContext({super.key, required super.child});

  @override
  ProviderContextElement createElement() => ProviderContextElement(this);

  @override
  bool updateShouldNotify(covariant ProviderContext oldWidget) {
    return false;
  }
}

class ProviderContextElement extends InheritedElement {
  ProviderContextElement(super.widget);

  @override
  bool get clearDependencyOnRebuild => true;

  final _container = ProviderContainer();

  @override
  Map<ProviderListenable, ProviderSubscription<Object?>>? getDependencies(
      Element dependent) {
    return super.getDependencies(dependent)
        as Map<ProviderListenable, ProviderSubscription<Object?>>?;
  }

  T _watch<T>(ProviderListenable<T> provider, Element dependent) {
    dependent.dependOnInheritedElement(this, aspect: provider);

    return getDependencies(dependent)![provider]!.read() as T;
  }

  @override
  @protected
  void updateDependencies(
    Element dependent,
    covariant ProviderListenable<Object?> key,
  ) {
    var deps = getDependencies(dependent);

    if (deps == null) {
      deps = {};
      setDependencies(dependent, deps);
    }

    deps.putIfAbsent(key, () {
      void listener(Object? prev, Object? next) => dependent.markNeedsBuild();

      final sub = _container.listen<Object?>(key, listener);
      return sub;
    });
  }

  @override
  @protected
  void removeDependencies(Element dependent) {
    final subs = getDependencies(dependent);
    if (subs != null) {
      super.removeDependencies(dependent);
      for (final sub in subs.values) {
        sub.close();
      }
    }
  }

  @override
  void unmount() {
    _container.dispose();
    super.unmount();
  }
}

///
extension RiverpodContext on BuildContext {
  T watch<T>(ProviderListenable<T> provider) {
    final element = getElementForInheritedWidgetOfExactType<ProviderContext>()!
        as ProviderContextElement;

    return element._watch(provider, this as Element);
  }

  T read<T>(ProviderBase<T> provider) {
    final element = getElementForInheritedWidgetOfExactType<ProviderContext>()!
        as ProviderContextElement;

    return element._container.read(provider);
  }
}

I have tested this code and it achieves the desired behavior, which is preserving Riverpod features while supporting raw StatelessWidgets (instead of having to use a custom subclass of Widget)

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Hello @goderbauer!
How likely do you think it is for this PR to get merged?

I'd like to know if I can confidently assume that this will be merged at some point, as this change is pivotal for Riverpod.
If this gets accepted, I have a lot of work to do related to this. So this decision is a bit of a blocker for me

How likely do you think it is for this PR to get merged?

Since InheritedWidgets are a very basic building block of the Flutter framework, I'd like to get the opinion from other team members on this one as well before making a decision. I'll get back to you.

I'd like to get the opinion from other team members on this one as well before making a decision. I'll get back to you.

Thanks!

For some reason, github doesn't allow me to answer your comment in the comment section, so copy-pasting it here:

Protected is defined as:

Used to annotate an instance member in a class or mixin which is meant to be visible only within the declaring library, > > and to other instance members of the class or mixin, and their subtypes.

https://pub.dev/documentation/meta/latest/meta/protected-constant.html

Isn't that exactly what we want here?

My comment refers to how the current use-case isn't a subtype, but rather a base type.

Instead of Element exposing this property, then InheritedElement using it, it's the opposite: InheritedElement defines it, and Element uses it.

But it's probably not worth spending too much time on this. I'll add the annotation

After thinking about this some more: This PR basically implements two separate features:

1. InheritedElements can observe that a dependent was removed
2. Dependancies can be configured to be cleared on rebuild

Are these features only useful for you as a pair? The changes for 1) are pretty uncontroversial. So, if those alone are useful to you, you could split those out in a separate PR and we can get that one approved pretty quickly.

The second feature is a little more controversial as it fundamentally changes the contract around inherited widgets, which creates some usability concerns (can't be used from didChangeDependencies) and also requires some more careful thinking about edge cases (e.g. LayoutBuilder).

I need them in pair, yes.

About didChangeDependencies, it's possible that there is a way to support it.
If supporting didChangeDependencies is necessary, maybe we could keep track of the dependencies used within didChangeDependencies and not clear them until didChangeDependencies is re-executed?
It'd need some considerations because State.didChangeDependenciesis executed on Element.performRebuild instead of Element.didChangeDependencies. But it's probably doable

But I'll admit, I don't care about didChangeDependencies. My packages have had various features that are incompatible with didChangeDependencies for years, and I haven't received complaints.

In a world where I update this PR to support didChangeDependencies, would this PR be accepted then?
I won't need it. But if that's necessary, I can look into it.

I wouldn't want to spend too much time on supporting that if that changes nothing to the outcome though.

As someone that has implemented a custom InheritedElement before, this change is a nice way to know when dependents stop listening. Without it, we have no way of cleaning up resources or stop notifying dependents that are no longer using our InheritedWiget during build.

While this solution doesn't result in a simple and easy to use API, it seems like the only way to do it without also regressing performance in the average use case of InheritedWiget.

After some more deliberation I still don't think we should do this. Element is such a core class of the framework. If anything, we should simplify it and not add more complexity to it. Also, as expressed earlier, I don't like how this makes the inherited widget API less predictable from the side of the callee. It is now up to the inherited widget to decide how the dependency will behave and it will not behave the same from all potential call sites (didChangeDependencies vs. build).

Also, the use case that was mentioned somewhere of disposing resources when nobody is listening anymore can be implemented on top of the existing mechanism: Instead of vending the resource directly from the of call, a manager object can be returned, from which interested parties can obtain a handle. When you don't need the resource anymore, you dispose the handle. When all handles are disposed, the resource can be freed.

Also, the use case that was mentioned somewhere of disposing resources when nobody is listening anymore can be implemented on top of the existing mechanism: Instead of vending the resource directly from the of call, a manager object can be returned, from which interested parties can obtain a handle. When you don't need the resource anymore, you dispose the handle. When all handles are disposed, the resource can be freed.

An object with a dispose function is fundamentally incompatible with this proposal.

The core goal behind this API is to simplify the logic in consumers as much as possible.
With this proposal, consumers don't need to deal with any dispose logic. With yours, this re-add the need for consumers to free resources

It is now up to the inherited widget to decide how the dependency will behave

Could you clarify? I don't understand how that's different from existing InheritedWidgets.

it will not behave the same from all potential call sites (didChangeDependencies vs. build).

As I mentioned before, I'm fairly sure we could get didChangeDependencies to work.
I just personally believe that didChangeDependencies is a flawed life-cycle, so I don't care about it. But if necessary, I can do the work to support it.

It is now up to the inherited widget to decide how the dependency will behave

Could you clarify? I don't understand how that's different from existing InheritedWidgets.

The InheritedWidget decides whether a dependency is cleared or not, that's different from how InheritedWidget behaves today.

The InheritedWidget decides whether a dependency is cleared or not

The removeDependency method was marked with @mustCallSuper as you requested, so that shouldn't be the case. Whenever the method is called (which is in control of the consumer), the dependency should be removed.

if you want, we could add an assert in the call site of removeDependent to verify that after the invocation, the dependency was indeed removed.

Maybe something among the lines of:

element.removeDependent(this);
assert(!element._dependents.contains(this), 'Bad state, the dependentwas not removed');

I was talking about the clearDependencyOnRebuild flag.

As I said before, I am totally fine with exposing removeDependency. My issue is with adding the clear flag and the associated complexity.

An alternate proposal I made was to add the logic on dependOnInheritedWidget

What about: context.dependOnInheritedWidget(clearOnRebuild: true)? Would you be fine with this? I don't mind either way, as I'm in control of both the InheritedElement and the invocation of dependOnInheritedWidget

I implemented this PR this way because having the logic on the InheritedElement means no newcomer would come accross this logic.
I'm fine with putting the logic on the consumer, but this would expose the API to newcomers

I also acknowledge that the problem of not clearing dependencies is a real issue and one that we should ideally fix. For the reasons outlined in my other comments, I don't think this here is the right fix.

The library has to adapt to Flutter and not the other way around. I find this approach very dangerous for the reasons cited above. I believe the Flutter team will make the best decision, but I don't agree with this PR.

I dont think it's good idea the flutterx change to adapt to this riverpod! Changing the tree this way it will cause the same problem the dom of browsers had had in 2010 when the dom was unlock to be manipulated direclly with whatever we want and the result before was crashes, unexpected render bugs, poor performance. To solve this the tools like react virtual doom came with reconciliation algorithm to solve a problem we wont have if the architetural limit had have mantained. For me this case it's the same. Today it's this riverpod, tomorrow it's another package ou another over engenering to solve futures problems into core elements of flutter wich the cause was this kind modifications.

The problem is by no means specific to Riverpod. I explained it that way, because that's the most specific reason why I need this change.

To avoid the "x/y problem".

The ecosystem will benefit it as a whole, such as SharedAppData being enabled to do more.

There's a design doc will cover that in due time. I'll close this PR while I'm working on the docs

Context is Flutter's spine and it should be safe-guarded from devs (we've seen what devs can do when they started manipulating DOM as @pauloantonelli mentioned).

So, my question is: once we unlock this door, can we GUARANTEE that devs won't be able to f* up the Context integrity?

I won't be surprised if in six months from now we start seeing apps crashing "randomly" just because some nice package is messing around with the context in a way that "nobody haven't predicted"!

Context should not be allowed to be changed as simple as that. Actually, Flutter should not be changed as simple as that just to adapt to a package needs. I think it should be the opposite.

Unfortunately, I'm not able to see the "ecosystem benefits" as you mentioned (perhaps we need more clarifications). What I see instead is a breach that will let devs do things that they shouldn't be doing.

I spend hours and hours on the discord helping dozens of devs a day "fixing" problems they wouldn't had if they just stop using these "silver-bullet" packages. I teach them to use what we get natively because it is so SIMPLE to use.

I see a lot of packages trying to change the way Flutter was originally designed to work, forcing some architectures with a bunch of unnecessary overengineering, when at the end of the day ALL can be done in an elegant way, just by using what Flutter's creators designed originally: setState/changeNotifier/inheritedWidget.

(Please let me highlight that is just my opinion and my opinion only.)

Well, leaving my personal opinion aside, I think these changes should not be applied until we have more deep discussions with the community, 'cos we really need to be aware of all its benefits and side effects.

I don't understand what's up with y'all.

This PR is about fixing a Flutter "bug" that's been around for a long time (widgets never stopping to listen to an InheritedWidget).
Riverpod is mentioned simply to explain why I personally need this bug fixed.

The team itself agreed that fixing this bug would be valuable (#107112 (comment))
We can debate whether the proposed implementation is the correct one. But this change is not something you should be afraid of. It's absolutely not comparable to allowing DOM manipulation.

Correct me if I'm wrong, but I believe those comments are coming from the following video: https://www.youtube.com/watch?v=UQROTqpX0RM

It appears that the way the video described this PR is incorrect.
This PR does not involve inserting Elements from outside the widget tree at all. It doesn't allow adding/removing Elements.

It's about fixing an issue where if a widget does:

Widget build(context) {
  if (condition) {
    Theme.of(context);
  }
}

and condition changes from true to false, the widget would still rebuild if Theme changes.

So this PR has nothing in common with DOM manipulation. Chances are it's probably just a big misunderstanding.

Corrija-me se eu estiver errado, mas acredito que esses comentários estão vindo do seguinte vídeo: https://www.youtube.com/watch?v=UQROTqpX0RM

Parece que a maneira como o vídeo descreveu esta RP está incorreta. Esta RP não envolve inserir elementos de fora da árvore de widget em tudo. Não permite adicionar/remover elementos.

Trata-se de corrigir um problema onde se um widget faz:

Widget build(context) {
  if (condition) {
    Theme.of(context);
  }
}

e muda de verdadeiro para falso, o widget ainda se reconstruiria se mudasse.condition``Theme

Então esta RP não tem nada em comum com a manipulação do DOM. As chances são de que seja apenas um grande mal-entendido.

Thanks for watching Flutterando's video.
I'm just replaying the news.
Really, that's not the end goal.

I make myself available for any possible repair, if needed, using another video explaining better what happened.

That said, I still don't agree to change something from the core to serve a package. I'm also against replacing Provider with Riverpod.

Thanks, I appreciate the offer for a correction.
I don't think a new video that's necessary. Hopefully, the new people joining this PR thread will see the last few comments and understand

I don't like that some people decided to review-brigade the PR. But I don't think you're responsible for that. So all is well 😄