Skip to content
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

[go_router] 5.2.0 push doesn't update the URL anymore. #115962

Closed
ValentinVignal opened this issue Nov 24, 2022 · 8 comments · Fixed by flutter/packages#2904
Closed

[go_router] 5.2.0 push doesn't update the URL anymore. #115962

ValentinVignal opened this issue Nov 24, 2022 · 8 comments · Fixed by flutter/packages#2904
Assignees
Labels
c: regression It was better in the past than it is now found in release: 3.3 Found to occur in 3.3 found in release: 3.6 Found to occur in 3.6 has reproducible steps The issue has been confirmed reproducible and is ready to work on p: go_router The go_router package P2 Important issues not at the top of the work list package flutter/packages repository. See also p: labels. r: fixed Issue is closed as already fixed in a newer version

Comments

@ValentinVignal
Copy link
Contributor

Steps to Reproduce

  1. Execute flutter run on the code sample (see "Code sample" section below)
  2. Click on the button "Settings" to push the settings page
  3. Notice the URL didn't change when using go_router 5.2.0

Expected results:

I expect the URL to change

Actual results:

The URL didn't change


The issue doesn't happen in the previous version (5.1.10) so I believe this was introduced in flutter/packages#2786

I don't know if this is related to #99112 (comment) which plans to remove imperative methods. But since I didn't see anything in the changelogs or documentation, I'm assuming this is a regression.

Code sample

Or you can checkout https://github.com/ValentinVignal/flutter_app_stable/tree/go-router/push-no-url-change

import 'package:flutter/material.dart';
import 'package:go_router/go_router.dart';

void main() {
  runApp(const MyApp());
}

final router = GoRouter(
  routes: [
    GoRoute(
      path: '/',
      builder: (context, state) => const Home(),
    ),
    GoRoute(
      path: '/settings',
      builder: (context, state) => const Settings(),
    ),
  ],
);

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return MaterialApp.router(
      routerConfig: router,
    );
  }
}

class Home extends StatelessWidget {
  const Home({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Home'),
      ),
      body: Center(
        child: ElevatedButton(
          onPressed: () {
            router.push('/settings');
          },
          child: const Text('Settings'),
        ),
      ),
    );
  }
}

class Settings extends StatelessWidget {
  const Settings({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Settings'),
      ),
    );
  }
}
Logs
Launching lib/main.dart on Chrome in debug mode...
Waiting for connection from debug service on Chrome...             25.8s
This app is linked to the debug service: ws://127.0.0.1:51507/t8SpI7d7848=/ws
Debug service listening on ws://127.0.0.1:51507/t8SpI7d7848=/ws

💪 Running with sound null safety 💪

🔥  To hot restart changes while running, press "r" or "R".
For a more detailed help message, press "h". To quit, press "q".

An Observatory debugger and profiler on Chrome is available at: http://127.0.0.1:51507/t8SpI7d7848=
Flutter Web Bootstrap: Auto
The Flutter DevTools debugger and profiler on Chrome is available at:
http://127.0.0.1:9101?uri=http://127.0.0.1:51507/t8SpI7d7848=
Analyzing flutter_app_stable...                                         
No issues found! (ran in 4.1s)
[✓] Flutter (Channel stable, 3.3.8, on macOS 11.6.8 20G730 darwin-x64, locale en-GB)
    • Flutter version 3.3.8 on channel stable at /Users/valentin/flutter/flutter
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision 52b3dc25f6 (2 weeks ago), 2022-11-09 12:09:26 +0800
    • Engine revision 857bd6b74c
    • Dart version 2.18.4
    • DevTools version 2.15.0

[✓] Android toolchain - develop for Android devices (Android SDK version 30.0.3)
    • Android SDK at /usr/local/Caskroom/android-sdk/4333796
    • Platform android-33, build-tools 30.0.3
    • Java binary at: /Applications/Android Studio.app/Contents/jre/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 11.0.11+0-b60-7590822)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 13.2.1)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Build 13C100
    • CocoaPods version 1.11.3

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 2021.1)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 11.0.11+0-b60-7590822)

[✓] VS Code (version 1.73.1)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.52.0

[✓] Connected device (2 available)
    • macOS (desktop) • macos  • darwin-x64     • macOS 11.6.8 20G730 darwin-x64
    • Chrome (web)    • chrome • web-javascript • Google Chrome 107.0.5304.110

[✓] HTTP Host Availability
    • All required HTTP hosts are available

• No issues found!
Videos
5.2.0 5.1.10
Screen.Recording.2022-11-24.at.10.20.44.AM.mov
Screen.Recording.2022-11-24.at.10.17.18.AM.mov
@exaby73 exaby73 added the in triage Presently being triaged by the triage team label Nov 24, 2022
@exaby73
Copy link
Member

exaby73 commented Nov 24, 2022

Triage report

I can reproduce this issue. I can also confirm that the URL does indeed change in 5.1.10 but not in 5.2.0. Labeling as regression

Code Sample (Same as OP)
import 'package:flutter/material.dart';
import 'package:go_router/go_router.dart';

void main() {
  runApp(const MyApp());
}

final router = GoRouter(
  routes: [
    GoRoute(
      path: '/',
      builder: (context, state) => const Home(),
    ),
    GoRoute(
      path: '/settings',
      builder: (context, state) => const Settings(),
    ),
  ],
);

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return MaterialApp.router(
      routerConfig: router,
    );
  }
}

class Home extends StatelessWidget {
  const Home({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Home'),
      ),
      body: Center(
        child: ElevatedButton(
          onPressed: () {
            router.push('/settings');
          },
          child: const Text('Settings'),
        ),
      ),
    );
  }
}

class Settings extends StatelessWidget {
  const Settings({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Settings'),
      ),
    );
  }
}

@exaby73 exaby73 added c: regression It was better in the past than it is now p: first party package flutter/packages repository. See also p: labels. has reproducible steps The issue has been confirmed reproducible and is ready to work on p: go_router The go_router package found in release: 3.3 Found to occur in 3.3 found in release: 3.6 Found to occur in 3.6 and removed in triage Presently being triaged by the triage team labels Nov 24, 2022
@vinay416
Copy link

Yeah I'm facing the same issue when pushing the route URl is not updating. In mobile app using context.go going to the route but once I back to previous route then I can't longer go to same route again.

@s9th
Copy link

s9th commented Nov 27, 2022

context.pop() also doesn't update URL. Maybe it's a consequence, maybe it's the same root cause.

@tolo
Copy link

tolo commented Nov 29, 2022

It's related to the changes in RouteMatchList in 5.2.0. Previously, the location was calculated based on the RouteMatches, like this:

Uri get location =>
      _matches.isEmpty ? Uri() : Uri.parse(_matches.last.fullUriString);

But in 5.2.0, location was replaced with an uri field (final Uri uri) that is only set at the initial creation of the RouteMatchList, and never updated when pushing/popping.

@stuartmorgan stuartmorgan added the P2 Important issues not at the top of the work list label Nov 29, 2022
@tolo
Copy link

tolo commented Nov 29, 2022

Reading the discussion in #99112 I realise there are more complexity to consider, and I sort of understand the reasoning around that pushed routes perhaps doesn't justify a location change. However, I think pop is a bit different, since that operation is often performed indirectly via BackButtons etc (perhaps mostly mobile though, but perhaps not only), and it's performed for both pushed and "go:ed" routes. Perhaps the pop method in RouterDelegate needs to take into account if the last RouteMatch was an ImperativeRouteMatch or not, and apply different handling?

bors bot added a commit to get10101/10101-PoC that referenced this issue Nov 30, 2022
448: Revert "chore(deps): bump go_router from 5.1.5 to 5.2.0" r=da-kami a=da-kami

Reverts #422

fixes #441

Apparently we hit a regression bug that was already reported: flutter/flutter#115962 (comment)

Note: I did go through the changelog before merging this and I did not see any indicator that we would run into trouble. Reverting for now, as there is no solution for `0.5.2` reported yet.

Note: Before I found the regression I actually gave replacing `/` with a specific `/wallet` route (and setting `/wallet` as initial route) a try, because in `/cfd-trading` and it's subroutes we did not have this problem even with `0.5.2`. But I experienced the same behavior.

Co-authored-by: Daniel Karzel <D.Karzel@gmail.com>
@AceChen1
Copy link

AceChen1 commented Dec 5, 2022

@chunhtai may i know any status update or any schedule plan about this issue?

@chunhtai
Copy link
Contributor

chunhtai commented Dec 5, 2022

I plan to work on this soon.

@chunhtai chunhtai self-assigned this Dec 5, 2022
@exaby73 exaby73 added the r: fixed Issue is closed as already fixed in a newer version label Dec 8, 2022
@github-actions
Copy link

github-actions bot commented Mar 5, 2023

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: regression It was better in the past than it is now found in release: 3.3 Found to occur in 3.3 found in release: 3.6 Found to occur in 3.6 has reproducible steps The issue has been confirmed reproducible and is ready to work on p: go_router The go_router package P2 Important issues not at the top of the work list package flutter/packages repository. See also p: labels. r: fixed Issue is closed as already fixed in a newer version
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

8 participants