Document super call order for State.didChangeDependencies - #185945
Conversation
github-actions
Bot
added
the
framework
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds documentation to the didChangeDependencies method in framework.dart, advising that implementations should call the inherited method. A review comment suggests formatting the method reference using square brackets instead of backticks and omitting parentheses to align with Flutter's documentation style and enable dartdoc linking.
| /// Implementations of this method should start with a call to the inherited | ||
| /// method, as in `super.didChangeDependencies()`. |
There was a problem hiding this comment.
To maintain consistency with other lifecycle methods in this file (such as initState and didUpdateWidget) and to adhere to the Flutter documentation style, please use square brackets for the method reference instead of backticks. This also enables dartdoc to generate a clickable link to the inherited method. Additionally, the convention in this file is to omit the parentheses when referring to the method in this context.
/// Implementations of this method should start with a call to the inherited
/// method, as in [super.didChangeDependencies].References
- The Flutter style guide and Effective Dart recommend using square brackets to refer to identifiers in doc comments to enable linking and maintain a consistent look across the framework's documentation. (link)
There was a problem hiding this comment.
Thank you for the update. I have verified the changes and they look correct and follow the requested style guide.
There was a problem hiding this comment.
Good catch, switched it over in e8fc173. Now reads [super.didChangeDependencies] so dartdoc renders the link, and dropped the parens to match what initState and didUpdateWidget do right above. Should be consistent with the rest of the file now 🙌
5989016 to
e8fc173
Compare
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the issue-comment-reply. Please try again in a few hours by commenting |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM but I'm curious if anyone has a good reason why the super call should be first.
| /// Implementations of this method should start with a call to the inherited | ||
| /// method, as in [super.didChangeDependencies]. |
There was a problem hiding this comment.
Can anyone think of a good reason why it should be called first? I browsed the code hoping to find a definitive reason but I don't see one. The framework doesn't seem to do much itself with didChangeDependencies. See @navaronbracke's comment #28925 (comment).
That said, I'm all for providing some guidance and consistency for people here. If we can't find a definitive reason, maybe we should just add "typically", like: "...should typically start with a call..."
There was a problem hiding this comment.
I agree didChangeDependencies should generally be called first.
Can anyone think of a good reason why it should be called first?
Your State might mix in RestorationMixin. If your didChangeDependencies doesn't call super first, your didChangeDependencies implementation can read a stale restoration bucket.
There was a problem hiding this comment.
Thank you for the great example, that's what I was looking for. So then I think we can ignore my "typically" suggestion and keep the PR as-is.
loic-sharma
left a comment
loic-sharma
left a comment
There was a problem hiding this comment.
Thanks for the wonderful contribution!
justinmc
added
CICD
|
@fahaddoc Can you rebase this PR? Sorry I think it needs it to pass CI. |
Mirror the guidance already present on State.initState so overrides know to start with super.didChangeDependencies(). Doc-only change. Fixes flutter#28925.
Replace `super.didChangeDependencies()` with [super.didChangeDependencies] so dartdoc renders it as a proper link, matching the convention used elsewhere in framework.dart.
49ae1d4 to
9bef138
Compare
|
Done, rebased and force-pushed. Thanks @justinmc 🙏 |
| /// expensive to do for every build. | ||
| /// | ||
| /// Implementations of this method should start with a call to the inherited | ||
| /// method, as in [super.didChangeDependencies]. |
There was a problem hiding this comment.
It looks like CI checks fail because Dartdoc is unhappy with this syntax: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8674822439837403393/+/u/run_test.dart_for_docs_shard_and_subshard_None/stdout
dartdoc:stdout: Generating docs for package flutter...
dartdoc:stderr: error: unresolved doc reference [super.didChangeDependencies]
dartdoc:stderr: from popup_menu.PopupMenuButtonState.didChangeDependencies: (file:///b/s/w/ir/x/w/flutter/packages/flutter/lib/src/material/popup_menu.dart:1600:8)
dartdoc:stderr: in documentation inherited from framework.State.didChangeDependencies: (file:///b/s/w/ir/x/w/flutter/packages/flutter/lib/src/widgets/framework.dart:1485:8)
We can fix this by switching to backticks:
Dartdoc cannot resolve [super.didChangeDependencies] when the doc comment is inherited by subclasses such as PopupMenuButtonState, which fails the docs shard with "unresolved doc reference". Switch back to a code span, matching how initState, didUpdateWidget, dispose and the other lifecycle methods in this file refer to their super calls.
justinmc
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
…12420) Manual roll requested by stuartmorgan@google.com flutter/flutter@b766512...27b0988 2026-08-05 kevmoo@users.noreply.github.com reland(tool): remove redundant --enable-experiment=record-use flag (flutter/flutter#190591) 2026-08-05 chris@bracken.jp Windows: Propagate enabled accessibility state (flutter/flutter#190507) 2026-08-05 engine-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from ltbuIH9Z3T_yOuigu... to vcANVO8VIDQHasH1X... (flutter/flutter#190589) 2026-08-05 256906086+mvincentong@users.noreply.github.com Document frozen embedder API structs (flutter/flutter#186842) 2026-08-05 49402500+fahaddoc@users.noreply.github.com Document super call order for State.didChangeDependencies (flutter/flutter#185945) 2026-08-05 dkwingsmt@users.noreply.github.com Move examples of `flutter/widgets` widgets out from `flutter/material` (flutter/flutter#189532) 2026-08-05 93888664+ColeSpringer@users.noreply.github.com [web] Use thread local strike caches in skwasm (flutter/flutter#190048) 2026-08-05 43089218+chika3742@users.noreply.github.com doc: fix typo in see also section for PrimaryScrollController.maybeOf (flutter/flutter#190386) 2026-08-05 jason-simmons@users.noreply.github.com Migrate the shell unit tests from legacy Dart native functions to FFI (flutter/flutter#190473) 2026-08-05 47866232+chunhtai@users.noreply.github.com render proxy box now defaults baseline calculation to null (flutter/flutter#190269) 2026-08-05 36861262+QuncCccccc@users.noreply.github.com Update Widgets Localizations from Translation Console (flutter/flutter#190503) 2026-08-04 154381524+flutteractionsbot@users.noreply.github.com Revert: fix(tool): remove redundant --enable-experiment=record-use flag (flutter/flutter#190583) 2026-08-04 engine-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from 1frGe_KltAJKkeyPg... to ltbuIH9Z3T_yOuigu... (flutter/flutter#190561) 2026-08-04 chris@bracken.jp iOS,macOS: add tsan and ubsan support for Swift (flutter/flutter#190497) 2026-08-04 34465683+rkishan516@users.noreply.github.com fix: update on_message_ to nullptr after window destroy so that dart gets destroy message (flutter/flutter#185807) 2026-08-04 bkonyi@google.com [flutter_tools] Gracefully handle locked Windows files during clean (flutter/flutter#190095) 2026-08-04 chris@bracken.jp iOS,macOS: make Logger thread-safe, conform to Sendable (flutter/flutter#190488) 2026-08-04 chris@bracken.jp iOS: Eliminate use of IOSContextNoop in platform view tests (reland) (flutter/flutter#190509) 2026-08-04 kevmoo@users.noreply.github.com fix(tool): remove redundant --enable-experiment=record-use flag (flutter/flutter#190475) 2026-08-04 awolff@google.com android_hardware_smoke_test: Detect blank image failures or EGL initialization warnings and retry (flutter/flutter#190110) 2026-08-04 30870216+gaaclarke@users.noreply.github.com Bumps text gamma on windows to match skia. (flutter/flutter#190477) 2026-08-04 engine-flutter-autoroll@skia.org Roll Skia from 48b58ee222f1 to a8583a0a2c11 (2 revisions) (flutter/flutter#190537) 2026-08-04 kevmoo@users.noreply.github.com [tool][web] Intercept dart2wasm errors & append JS migration footers (flutter/flutter#190476) 2026-08-04 dacoharkes@google.com [record_use] Migrate IconTreeShaker to `package:record_use` (flutter/flutter#190225) 2026-08-04 s4bre.py@gmail.com Handle unexpected exceptions during Azure metadata detection (flutter/flutter#189457) 2026-08-04 15619084+vashworth@users.noreply.github.com Fix merge conflict from flutter/flutter#190369 (flutter/flutter#190544) 2026-08-04 15619084+vashworth@users.noreply.github.com Prepare device support symbols (flutter/flutter#190369) 2026-08-04 engine-flutter-autoroll@skia.org Roll Packages from ac87e65 to 3498b9d (1 revision) (flutter/flutter#190532) 2026-08-04 engine-flutter-autoroll@skia.org Roll Skia from a08d918ebd6a to 48b58ee222f1 (11 revisions) (flutter/flutter#190527) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC stuartmorgan@google.com,tarrinneal@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
5 participants
The doc comment on
State.didChangeDependenciesis silent about where overrides should place thesuper.didChangeDependencies()call, even though@mustCallSuperis set on it. The sibling lifecycle hookState.initStatealready documents this convention, so the two methods read inconsistently and contributors hittingdidChangeDependenciesfor the first time have to guess.This PR adds a short note to the dartdoc that mirrors the wording on
initState, telling implementations to start with a call to the inherited method (super.didChangeDependencies()). It is a documentation-only change topackages/flutter/lib/src/widgets/framework.dart. There is no behavior change, no API change, and no new public surface.Fixes #28925.
Pre-launch Checklist
///).