-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[go_router] Fix Popping state and re-rendering scaffold at the same time doesn't update the URL on web #150312 #6953
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
| }()); | ||
|
|
||
| routerDelegate.pop<T>(result); | ||
| routeInformationProvider.popSave<T>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may not work if the routerDelegate.pop removes pageless route such as dialog or modalbottomroute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've try use go_router 14.2.0 of pub.dev and run demo follwing code also not work, I thought it was normal, see the picture above.
if misundestand please tell me how reproduce it.
demo code:
import 'dart:async';
import 'package:flutter/material.dart';
import 'package:go_router/go_router.dart';
class CounterStream {
int _counter = 0;
final _streamController = StreamController<int>.broadcast();
Stream<int> get stateStream => _streamController.stream.asBroadcastStream();
void increment() {
_streamController.sink.add(++_counter);
}
Future<void> delayedRerender() async {
increment();
increment();
}
void dispose() {
_streamController.close();
}
}
final CounterStream counterStream = CounterStream();
class StreamListener extends ChangeNotifier {
StreamListener(Stream<dynamic> stream) {
notifyListeners();
_subscription = stream.asBroadcastStream().listen((_) {
notifyListeners();
});
}
late final StreamSubscription<dynamic> _subscription;
@override
void notifyListeners() {
super.notifyListeners();
print('refreshing the router');
}
@override
void dispose() {
_subscription.cancel();
super.dispose();
}
}
void main() {
GoRouter.optionURLReflectsImperativeAPIs = true;
WidgetsFlutterBinding.ensureInitialized();
runApp(const MyApp());
}
class MyApp extends StatefulWidget {
const MyApp({super.key});
@override
State<MyApp> createState() => _MyAppState();
}
final _rootNavigatorKey = GlobalKey<NavigatorState>();
final GoRouter _router = GoRouter(
initialLocation: '/',
navigatorKey: _rootNavigatorKey,
refreshListenable: StreamListener(counterStream.stateStream),
routes: <RouteBase>[
ShellRoute(
builder: (context, state, child) {
return GenericPage(child: child);
},
routes: [
GoRoute(
path: '/',
builder: (BuildContext context, GoRouterState state) =>
const GenericPage(showPushButton: true, path: 'a'),
routes: [
GoRoute(
path: 'a',
name: 'a',
builder: (BuildContext context, GoRouterState state) =>
const GenericPage(showPushButton: true, path: 'b'),
routes: [
GoRoute(
path: 'b',
name: 'b',
builder: (BuildContext context, GoRouterState state) =>
const GenericPage(showBackButton: true),
),
],
),
],
),
],
),
],
);
class _MyAppState extends State<MyApp> {
late StreamSubscription<int> _stateSubscription;
int _currentState = 0;
@override
void initState() {
super.initState();
_stateSubscription = counterStream.stateStream.listen((state) {
setState(() {
_currentState = state;
});
});
}
@override
void dispose() {
_stateSubscription.cancel();
super.dispose();
}
@override
Widget build(BuildContext context) {
return MaterialApp.router(
title: 'Flutter Demo',
theme: ThemeData(
primarySwatch: Colors.blue,
),
routerConfig: _router,
);
}
}
class DialogTest extends StatelessWidget {
const DialogTest({super.key});
@override
Widget build(BuildContext context) {
return Material(
type: MaterialType.transparency,
child: Center(
child: Container(
color: Colors.white,
width: 300,
height: 300,
alignment: Alignment.center,
child: Column(
children: ['Navigator::pop', 'GoRouter::pop'].map((e) {
return InkWell(
child: Row(children: [
Text(e),
const Icon(Icons.close),
]),
onTap: () {
if (e == 'GoRouter::pop') {
GoRouter.of(context).pop();
} else {
Navigator.of(context).pop();
}
},
);
}).toList(),
),
),
),
);
}
}
class GenericPage extends StatefulWidget {
final Widget? child;
final bool showPushButton;
final bool showBackButton;
final String? path;
const GenericPage({
this.child,
Key? key,
this.showPushButton = false,
this.showBackButton = false,
this.path,
}) : super(key: key ?? const ValueKey<String>('ShellWidget'));
@override
State<GenericPage> createState() => _GenericPageState();
}
class _GenericPageState extends State<GenericPage> {
late StreamSubscription<int> _stateSubscription;
int _currentState = 0;
@override
void initState() {
super.initState();
_stateSubscription = counterStream.stateStream.listen((state) {
setState(() {
_currentState = state;
});
});
}
@override
void dispose() {
_stateSubscription.cancel();
super.dispose();
}
@override
Widget build(BuildContext context) {
return Scaffold(
appBar: widget.child != null
? AppBar(
title: Text('Count: $_currentState'),
actions: [
TextButton(
onPressed: () {
showDialog(
context: context,
builder: (context) {
return DialogTest();
},
);
},
child: const Text('dialog1'),
),
TextButton(
onPressed: () {
showModalBottomSheet(
context: context,
builder: (context) {
return DialogTest();
},
);
},
child: const Text('dialog2'),
)
],
)
: null,
body: _buildWidget(context));
}
Widget _buildWidget(BuildContext context) {
if (widget.child != null) {
return widget.child!;
}
if (widget.showBackButton) {
return TextButton(
onPressed: () {
// WHEN THE USER PRESSES THIS BUTTON, THE URL
// DOESN'T CHANGE, BUT THE SCREEN DOES
counterStream
.increment(); // <- when removing this line the issue is gone
GoRouter.of(context).pop();
},
child: const Text('<- Go Back'),
);
}
if (widget.showPushButton) {
return TextButton(
onPressed: () {
GoRouter.of(context).goNamed(widget.path!);
},
child: const Text('Push ->'),
);
}
return Text('Current state: $_currentState');
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not looked into it more, but we can't guarantee the goRouter.pop will pop the last routematch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can guarantee the goRouter.pop will pop the last routematch, because we just mark the RouteInformation in GoRouteInformationProvider, the pop will use navigatorKey to pop the router and use currentConfiguration of GoRouterDelegate to check last router and we have no change currentConfiguration GoRouterDelegate value.
pop of NavigatorState won't be affected either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant is that the pop can pop a dialog instead of the page. It is also possible that the page can contain localhistory, such as open drawer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seemed not addressed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will continue to look into this.
|
Hi @ahyangnb Are you still planning on coming back to this pr? |
|
|
Hi @ahyangnb I am going to close this pr for now, feel free to reopen once you have time |
|
hi, can we open it again? this pr is already tested by myself and no mistake with the code view |
…ime doesn't update the URL on web [new] (#8352) > the old pr #6953 Change the 3 files of lib and add a example, because issue flutter/flutter#150312 The reason for the issue is when call `counterStream.increment();` will notify `notifyListeners()` of `StreamListener`, and finally affect `_handleRouteInformationProviderNotification` of` _RouterState` in `packages/flutter/lib/src/widgets/router.dart`. When pop should not call `_handleRouteInformationProviderNotification` otherwise will be path wrong. because the value of `GoRouteInformationProvider` is not the latest. ## Solution: always make the value of `GoRouteInformationProvider` knew the latest `RouteInformation`[but not to do `notifyListeners()`]. i don't know if adding `pop` to `NavigatingType` is reasonable, if any place needs to be changed please tell me. ## Result: After changed, the issue been solved and all the others pop not be affect such as drawer/dialog/bottomsheet, See the video below. > flutter/flutter#150312 > exempt from version changes > this PR is exempt from CHANGELOG changes. > this PR is test-exempt. > the `go_route_test.dart` will error if `testWidgets('throw if redirect to itself')`[I have this problem when I pull the online branch. it's not relate this PR, All of the others test success, So I view this as all test cases successfully passing.] ``` Exception has occurred. _AssertionError ('package:go_router/src/parser.dart': Failed assertion: line 110 pos 18: '!matchList.last.route.redirectOnly': A redirect-only route must redirect to location different from itself. The offending route: GoRoute#0ae4e(name: null, path: "route", Redirect Only)) ```
…ime doesn't update the URL on web [new] (flutter#8352) > the old pr flutter#6953 Change the 3 files of lib and add a example, because issue flutter/flutter#150312 The reason for the issue is when call `counterStream.increment();` will notify `notifyListeners()` of `StreamListener`, and finally affect `_handleRouteInformationProviderNotification` of` _RouterState` in `packages/flutter/lib/src/widgets/router.dart`. When pop should not call `_handleRouteInformationProviderNotification` otherwise will be path wrong. because the value of `GoRouteInformationProvider` is not the latest. ## Solution: always make the value of `GoRouteInformationProvider` knew the latest `RouteInformation`[but not to do `notifyListeners()`]. i don't know if adding `pop` to `NavigatingType` is reasonable, if any place needs to be changed please tell me. ## Result: After changed, the issue been solved and all the others pop not be affect such as drawer/dialog/bottomsheet, See the video below. > flutter/flutter#150312 > exempt from version changes > this PR is exempt from CHANGELOG changes. > this PR is test-exempt. > the `go_route_test.dart` will error if `testWidgets('throw if redirect to itself')`[I have this problem when I pull the online branch. it's not relate this PR, All of the others test success, So I view this as all test cases successfully passing.] ``` Exception has occurred. _AssertionError ('package:go_router/src/parser.dart': Failed assertion: line 110 pos 18: '!matchList.last.route.redirectOnly': A redirect-only route must redirect to location different from itself. The offending route: GoRoute#0ae4e(name: null, path: "route", Redirect Only)) ```
…ime doesn't update the URL on web [new] (flutter#8352) > the old pr flutter#6953 Change the 3 files of lib and add a example, because issue flutter/flutter#150312 The reason for the issue is when call `counterStream.increment();` will notify `notifyListeners()` of `StreamListener`, and finally affect `_handleRouteInformationProviderNotification` of` _RouterState` in `packages/flutter/lib/src/widgets/router.dart`. When pop should not call `_handleRouteInformationProviderNotification` otherwise will be path wrong. because the value of `GoRouteInformationProvider` is not the latest. ## Solution: always make the value of `GoRouteInformationProvider` knew the latest `RouteInformation`[but not to do `notifyListeners()`]. i don't know if adding `pop` to `NavigatingType` is reasonable, if any place needs to be changed please tell me. ## Result: After changed, the issue been solved and all the others pop not be affect such as drawer/dialog/bottomsheet, See the video below. > flutter/flutter#150312 > exempt from version changes > this PR is exempt from CHANGELOG changes. > this PR is test-exempt. > the `go_route_test.dart` will error if `testWidgets('throw if redirect to itself')`[I have this problem when I pull the online branch. it's not relate this PR, All of the others test success, So I view this as all test cases successfully passing.] ``` Exception has occurred. _AssertionError ('package:go_router/src/parser.dart': Failed assertion: line 110 pos 18: '!matchList.last.route.redirectOnly': A redirect-only route must redirect to location different from itself. The offending route: GoRoute#0ae4e(name: null, path: "route", Redirect Only)) ```




Change the 3 file palce, because issue #150312
The reason for the issue is when call
counterStream.increment();will notifynotifyListeners()ofStreamListener, and finally affect_handleRouteInformationProviderNotificationof_RouterStateinpackages/flutter/lib/src/widgets/router.dart.When pop should not call
_handleRouteInformationProviderNotificationotherwise will be path wrong.because the value of
GoRouteInformationProvideris not the latest.solution:
always make the value of GoRouteInformationProvider knew the latest RouteInformation[but not to do
notifyListeners()].i don't know if adding pop to NavigatingType is reasonable, if any place needs to be changed please tell me.
Pre-launch Checklist
i was run
shell_route_test.dartto check the change I am making, all existing and new tests are passing.dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style], or this PR is [exempt from CHANGELOG changes].///).