[go_router] Detect both framework and material_ui/cupertino_ui apps for Hero controller - #12695
[go_router] Detect both framework and material_ui/cupertino_ui apps for Hero controller#12695yashas-hm wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates go_router to version 18.0.1, resolving an issue where Hero flight animations failed to play for nested routes inside shell routes when using Flutter's framework MaterialApp or CupertinoApp. The fix updates isCupertinoApp and isMaterialApp to check for both the framework-specific and package-specific variants of these widgets, and adds corresponding regression tests. The reviewer suggests reordering these checks to prioritize the framework-specific types first, allowing the lookup to short-circuit earlier for most applications and improve efficiency.
Piinks
left a comment
There was a problem hiding this comment.
go_router 18.0.0 migrated its Material/Cupertino helpers to the material_ui /
cupertino_ui packages. As part of that migration, isMaterialApp /
isCupertinoApp (in lib/src/pages/material.dart and lib/src/pages/cupertino.dart)
resolve MaterialApp / CupertinoApp to the material_ui / cupertino_ui types
and detect the app with findAncestorWidgetOfExactType.Because that is an exact type match, an app built with the framework's own
MaterialApp / CupertinoApp (from package:flutter/material.dart /
package:flutter/cupertino.dart) is no longer detected.
This is intentional, and the reason why the migration to material_ui and cupertino_ui of this package was a major release. We are not intending to try to support both flutter/material and material_ui in this package at the same time.
|
@Piinks Should it rather have some error instead of silently failing? I came across this issue after an update by a dependabot. |
I don't think so. Since the class names are all the same I am not sure how it could fail for this reason. This package uses semantic versioning, so the major release bump is meant to indicate that it is not backwards compatible. |
Thanks for pushing back, let me clarify, because this one is genuinely counterintuitive. The class names being identical is exactly why it breaks. findAncestorWidgetOfExactType matches on runtime type identity (ancestor.widget.runtimeType == T), not on the class's name. As of 18.0.0 these This isn't hypothetical the regression test reproduces it. Under a framework MaterialApp.router with a ShellRoute, on current main go_router installs a plain HeroController on both the root and the nested On semver: agreed that the 18.0.0 major bump signals breaking changes but the intended breakage is the SDK floor and go_router's internal migration to material_ui, not a requirement that every consumer |
|
The intention is that users will not migrate to 18.0.0 of go_router until they have migrated to material_ui. Supporting both libraries at the same time through material_ui is not something we want to do. material_ui and cupertino_ui do provide utilities for bridging the gap, allowing users to migrate to material_ui or cupertino_ui while their dependencies have not migrated yet. More details are in https://docs.flutter.dev/release/breaking-changes/material-ui-and-cupertino-ui Since this is not a change we want to make, I am going to close it. Thanks for contributing! |
Description
go_router 18.0.0 migrated its Material/Cupertino helpers to the
material_ui/cupertino_uipackages. As part of that migration,isMaterialApp/isCupertinoApp(inlib/src/pages/material.dartandlib/src/pages/cupertino.dart)resolve
MaterialApp/CupertinoAppto thematerial_ui/cupertino_uitypesand detect the app with
findAncestorWidgetOfExactType.Because that is an exact type match, an app built with the framework's own
MaterialApp/CupertinoApp(frompackage:flutter/material.dart/package:flutter/cupertino.dart) is no longer detected.RouteBuilderthen fallsthrough to a bare
HeroController()instead ofcreateMaterialHeroController()/createCupertinoHeroController(), and Hero animations silently stop flying insideShellRoute/StatefulShellRoute. There is no compile error and no warning — theanimation just disappears.
This is a transition-period problem: while Material and Cupertino are being decoupled
from the SDK (flutter/flutter#184093),
package:flutter/material.dartremains fullysupported and is what the vast majority of apps still use.
This PR makes app-type detection recognize both the framework's
MaterialApp/CupertinoAppand thematerial_ui/cupertino_uivariants, so thecorrect
HeroControlleris installed regardless of which Material/Cupertino librarythe app is built with. A code comment notes the framework check can be removed once
package:flutter/material.dartis sunset.Related Issues
Fixes flutter/flutter#192043
Context: flutter/flutter#184093 (Material/Cupertino decoupling)
Tests
createMaterialHeroController()/createCupertinoHeroController()for both aframework
MaterialApp/CupertinoAppand amaterial_ui/cupertino_uiMaterialApp/CupertinoApp.Pre-Review Checklist
[shared_preferences]///).