Avoid calling didChangeDependences on a State that has dropped out of the tree - #49527

It is a breaking change

This solves the "Avoid calling didChangeDependences on a State that has dropped out of the tree".
But it also debounce the call too.

If a StatefulWidget depends on two Inheritedwidgets, and both Inheritedwidgets changes within the same frame, then the StatefulWidget's didChangeDependencies is currently called twice
This change would make it called once.

Hm. Should be easy to unbreak that by using a count instead of a book.

But it seems like we should eventually break it.

whether it's a breaking change or not is just whether it breaks any of the contributed tests. (see our breaking change policy)

assuming it doesn't, this seems valuable.

I don't know about the contributed tests, but it does break some of the content available online.

I've seen didChangeDependencies used for pushing routes/modals which works only because it is executed synchronously on change.

@override
void didChangeDependencies() {
  super.didChangeDependencies();

  if (whatever) {
    showBottomSheet<void>(
      context: context,
      builder: (_) {
        return Text('Hello world');
      },
    );
  }
}

see https://flutter.dev/docs/resources/compatibility

I know about flutter/test, but it does not consider all the private apps, articles, and answers online.

I know this for fact because a few people asked me about "how to push Routes/... on provider change" and I remember answering a few of these with didChangeDependencies

Here's one for example: https://stackoverflow.com/questions/59400368/how-to-do-something-if-changenotifier-notifies-with-provider-package-similar
The author edited his question to mention that I answered him about didChangeDependencies (used as mentionned in #49527 (comment))

So even if all tests on Flutter and Flutter/test pass, there's still a risk here.
Making an announcement doesn't hurt anybody

This looks like a good change to me, but it is failing some tests on Cirrus.

Yeah, it's breaking some stuff with focus scope, trying to figure out why right now.

To make this work, I had to update FocusScope so that it does some work it used to only do in didChangeDependencies so that it now does that work also in deactivate.

I think this falls under:

Changes that do not break any tests. We will pretend that such changes are not breaking changes and make them without any effort to prevent breakages. In cases where we can imagine reasonable scenarios where developers would be affected negatively, by courtesy, once the change has landed, engineers are encouraged to announce the changes by sending an e-mail to flutter-announce and listing it on our Changelog wiki page (such that they will be included in our release notes). However, we do not consider these breaking changes.

I know about flutter/test, but it does not consider all the private apps, articles, and answers online.

I know this for fact because a few people asked me about "how to push Routes/... on provider change" and I remember answering a few of these with didChangeDependencies

@rrousselGit Thank you for considering this case - I'm among the ones who used that advice.

Would you mind suggesting an alternative that scenario?

This would only break you if your widget was dropping out of the tree AFAICT. If you didn't want to push a route in deactivate, you wouldn't want to have used the functionality this removes either, right?

I will be running this patch through a set of internal tests before landing - we have plenty of people using provider and if it breaks anything in there it's a breaking change.

This would only break you if your widget was dropping out of the tree AFAICT. If you didn't want to push a route in deactivate, you wouldn't want to have used the functionality this removes either, right?

No, this breaks a different situation unrelated to unmounting

It's about the lifecycle being called in a state where the tree is unlocked VS the tree being locked.

With such change, calling showBottomSheet in didChangeDependencies will now throw the typical "setState() or markNeedsBuild() called when widget tree was locked" when it didn't before

All in all, I'm definitely for this change.
If people need the old behavior, provider can implement something using a custom Element to help people migrate.

It's more about making sure that it's communicated, as I'm 99% sure that it may break someone's code

I know about flutter/test, but it does not consider all the private apps, articles, and answers online.

Feel free to submit tests that verify those (e.g. by creating a repo that just contains tests that reflect what those articles say, and submitting that repo to flutter/tests). That would be really useful in fact.

I ran this through all the internal tests and it passed.

I'm a bit torn on whether I should be trying to find a way to avoid having this be withint he build scope.

I know about flutter/test, but it does not consider all the private apps, articles, and answers online.

Feel free to submit tests that verify those (e.g. by creating a repo that just contains tests that reflect what those articles say, and submitting that repo to flutter/tests). That would be really useful in fact.

It shouldn't be my responsibility but either the authors of these articles/answers or some member of the Flutter team when triaging questions/issues.

I'm a bit torn on whether I should be trying to find a way to avoid having this be withint he build scope.

Here's a test that should fail with these changes:

import 'package:flutter/material.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:provider/provider.dart';

void main() {
  testWidgets('didChangeDependencies can modify parents', (tester) async {
    final child = MaterialApp(home: Scaffold(body: Example()));

    await tester.pumpWidget(Provider.value(value: 0, child: child));

    expect(find.text('bottom-sheet'), findsNothing);

    await tester.pumpWidget(Provider.value(value: 42, child: child));

    expect(find.text('bottom-sheet'), findsOneWidget);
  });
}

class Example extends StatefulWidget {
  @override
  _ExampleState createState() => _ExampleState();
}

class _ExampleState extends State<Example> {
  int previous;
  @override
  void didChangeDependencies() {
    super.didChangeDependencies();
    final value = Provider.of<int>(context);
    if (value != previous) {
      previous = value;
      if (value == 42) {
        showBottomSheet<void>(
          context: context,
          builder: (context) {
            return const Text('bottom-sheet');
          },
        );
      }
    }
  }

  @override
  Widget build(BuildContext context) {
    return Container();
  }
}

This test case seems really fishy to me.

Even without these changes, it will fail if you change the order of the condition so that it's true on the first build. With these changes, it will fail on both.

Without these changes, I'm confused about the expected behavior of

 await tester.pumpWidget(Provider.value(value: 42, child: const SizedBox()));

It does not show the modal, and does not error (?!).

I've tried a couple ways to disable the debug check in this case, but there's no clean way to do it so showing the modal works, because it wants to set state on other elements in the tree, and we can't figure all of that out without going through a build cycle, and by the time we know it's too late to be modifying other parts of the tree.

And given that this has several ways to fail, some of which just fail silently, it seems better to have it just error now rather than have it sometimes succeed and sometimes do nothing.

Proper solution is using yet another library? :(

didChangeDependencies is just like build, in that it shouldn't have side effects like showing a dialog. I'm surprised that doesn't assert, to be honest.

It asserts if:

• It's called on the first build
• It's called when the element is deactivated

It does not assert if:

• It's called on any non-first build where the element will stay in the tree.

But this PR will change that.

@SpajicM
A very simple fix would be to just wrap that code in a SchedulerBinding.instance.scheduleFrameCallback or maybe addPostFrameCallback - that's effectively what's happening when you show a dialog from a click handler.

IMO the proper fix should instead be in the line of what's done with Navigator 2.0, by making modals declarative, like I've done in https://github.com/rrousselGit/flutter_portal

That's off-topic though.
Should I make an issue about potentially merging that library in Flutter as part of Navigator 2.0, or is it pointless?

@rrousselGit - it would probably be good to file a new issue for that, maybe tag @chunhtai

I've added an answer to the linked stackoverflow question indicating why the behavior isn't safe to use this way and won't work going forward, and what to do about it. I'll also send a curteously email to flutter-announce.

After some thinking, I've realized that this PR broke flutter_hooks (not in a critical way).

While the behavior change is good, I think it broke the semantics of the life-cycles.

Before, we had a very clear separation:

• StateElement.didChangeDependencies called State.didChangeDependencies
• StateElement.build called State.build

But now things got entangled in a weird way, where StateElement.build is triggering both life-cycles on State.
That's why it broke these two packages.

My suggestion is:

Step 1: Revert to the old behavior of StateElement.didChangeDependencies calling State.didChangeDependencies.

Step 2: Introduce a new method: Element.markNeedsChangeDependencies.

Step 3: Migrate the previous didChangeDependencies call to markNeedsChangeDependencies instead.

This achieves the same behavior as right now, but leads to a more logical API and no longer flutter_hooks.
(or do #49506, which is what broke in flutter_hooks – but the team sounds against it)

Can you share some test cases that this breaks? If we revert we should add tests showing what it breaks.

Sure thing:

testWidgets('can decorate StatefulElement.build', (tester) async {
  await tester.pumpWidget(
    Provider.value(
      value: 0,
      child: HookWidget(),
    ),
  );

  // will crash inside didChangeDependencies
  await tester.pumpWidget(
    Provider.value(
      value: 42,
      child: HookWidget(),
    ),
  );
});

class HookWidget extends StatefulWidget {
  @override
  _HookWidgetState createState() => _HookWidgetState();

  @override
  _HookElement createElement() {
    return _HookElement(this);
  }
}

class _HookElement extends StatefulElement {
  _HookElement(StatefulWidget widget) : super(widget);
  static _HookElement _buildingElement;

  @override
  Widget build() {
    try {
      _buildingElement = this;
      final res = super.build();
      return res;
    } finally {
      _buildingElement = null;
    }
  }
}

class _HookWidgetState extends State<HookWidget> {
  @override
  void didChangeDependencies() {
    assert(_HookElement._buildingElement == null);
    Provider.of<int>(context);
    super.didChangeDependencies();
  }

  @override
  Widget build(BuildContext context) {
    assert(_HookElement._buildingElement != null);
    return Container();
  }
}

This breaks some health checks on flutter_hook (namely the #49506 flag I implemented myself) so nothing critical.

This is more about the implementation.

It feels off to have:

Widget build() {
  if (_didChangeDependencies) {
    _didChangeDependencies = false;
   didChangeDependencies();
  }
  return _state.build();
}

IMO it should be more something like:

void performRebuild() {
  ...
  Widget built;
  try {
    if (_didChangeDependencies) {
      _didChangeDependencies = false;
     didChangeDependencies();
    }
    built = build();
    debugWidgetBuilderValue(widget, built);
  }
  ...

This way we are not mixing build and didChangeDependencies

Am I understanding your test correctly by thinking it's basically the same as what you're requesting in #49505?

They are related.
It includes #49505, but also does extra things as it's not just a boolean, but an custom Element with a State and methods and stuff.

But yeah, the part that broke is the part where I use this static Element to implement a #49505

@rrousselGit I would like to encourage you to contribute your flutter_hooks tests to https://github.com/flutter/tests. That way, we will treat every change that breaks one of those tests as a breaking change and will work with you to mitigate the breakage (see our breaking change policy).

Looking at the code you posted above I don't fully understand the purpose of that test. You could just move the try/finally from _HookElement.build() to _HookWidgetState.build(), which would get you the old behavior even with this change (and as far as I can tell from the example above doesn't break anything else).

As it stands, I don't see any reason to revert this patch.

Actually flutter_mobx broke too – with a more important bug for this one (with basically the same test).

Looking at the code you posted above I don't fully understand the purpose of that test. You could just move the try/finally from _HookElement.build() to _HookWidgetState.build(), which would get you the old behavior even with this change (and as far as I can tell from the example above doesn't break anything else).

That's impossible.
Element is in the scope of the library, State is in the user scope.
We can't ask all users to include a try/finally inside their build method. And it's not feasible to refactor it into a State subclass either (and is a breaking change anyway).

And these answers miss the point. What do you think of:

void performRebuild() {
  ...
  Widget built;
  try {
    if (_didChangeDependencies) {
      _didChangeDependencies = false;
     didChangeDependencies();
    }
    built = build();
    debugWidgetBuilderValue(widget, built);
  }
  ...

Element is in the scope of the library, State is in the user scope.

That's not clear from the code you posted.

Anyhow, it looks like you found a solution.