Skip to content

Conversation

StevenSorial
Copy link
Contributor

second step of fixing #106790. First PR

This PR adds the routing methods .location, .go(context), .push(context), .pushReplacement(context), and replace(context) to GoRouteData. They will be overridden by the mixin generated by go_router_builder

Pre-Review Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Footnotes

  1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. 2 3

@StevenSorial
Copy link
Contributor Author

@hannah-hyj

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'It should throw beacuase there is not code generated',
'It should throw beacuase there is no code generated',

@chunhtai chunhtai requested a review from hannah-hyj May 19, 2025 16:54
@StevenSorial
Copy link
Contributor Author

@chunhtai @hannah-hyj Thanks for the approval. There are two things that I would appreciate some help with in this PR:

  • Currently if the user installed go_router >= 15.2.0, but go_router_builder < 3.0.0, the exception would be thrown on navigation. Is there a way to describe this dependency?
  • I don't like how the test I wrote looks 😅 Is there a way to improve it?

@AndrewDongminYoo
Copy link

Hi there,

I ran into a compilation error after upgrading to go_router_builder 3.0.0 together with go_router 15.1.3. The generated mixin looks like this:

mixin _$SplashRoute on GoRouteData {
  static SplashRoute _fromState(GoRouterState state) => const SplashRoute();

  @override
  String get location => GoRouteData.$location('/');

  @override
  void go(BuildContext context) => context.go(location);

  @override
  Future<T?> push<T>(BuildContext context) => context.push<T>(location);
  // …and so on
}

However, in go_router 15.1.3, GoRouteData does not declare location, go(), push(), etc., so the @override annotations cause errors:

Error: The method ‘location’ isn’t defined for the class ‘GoRouteData’.

It looks like the builder is already generating mixins for methods that are only available in an unreleased version of go_router. Could we either:

  1. Remove the @override annotations for methods not yet in GoRouteData, or
  2. Guard generation so that only methods present in the published go_router API are included?

Happy to provide a minimal repro if it helps. Thanks for looking into this!

@Piinks
Copy link
Contributor

Piinks commented Jun 10, 2025

Hi @StevenSorial thanks for contributing! What is the status of thisPR? Were you able to answer the questions you shared above? Looks like CI is happy, just some minor merge conflicts from the changelog and version bump.

@vware
Copy link

vware commented Jun 12, 2025

i dont mean to press this issue, but we now have 1000s of warnings of "The method doesn't override an inherited method.
Try updating this class to match the superclass, or removing the override annotation." in our projects... Could you please merge this? I don't think go_router_builder 3.0 should have been released without this PR here?

@StevenSorial
Copy link
Contributor Author

Hi @Piinks, what questions? I posted two questions for @chunhtai @hannah-hyj, but no one responded. I was hoping for my two PRs to get merged together, but unfortunately only one got merged and caused warnings to appear.

@Piinks
Copy link
Contributor

Piinks commented Jun 12, 2025

I posted two questions for @chunhtai @hannah-hyj

Thanks for sharing! I will see about getting answers here. :)

@chunhtai
Copy link
Contributor

chunhtai commented Jun 12, 2025

oh i didn't aware the go_router_builder pr needs go_router change, yes in this case this pr needs to be reviewed together with the go_router_builder pr and once both approved, go_router pr needs to merge in first.

Currently if the user installed go_router >= 15.2.0, but go_router_builder <3.0.0, the exception would be thrown on navigation. Is there a way to describe this dependency?

there is not much we can do except asking them to upgrade go_router_builder.

Copy link
Member

@hannah-hyj hannah-hyj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chunhtai chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 12, 2025
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 12, 2025
Copy link
Contributor

auto-submit bot commented Jun 12, 2025

autosubmit label was removed for flutter/packages/9277, because - The status or check suite Linux_android_legacy android_platform_tests_legacy_api_shard_5 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 12, 2025
Copy link
Contributor

auto-submit bot commented Jun 12, 2025

autosubmit label was removed for flutter/packages/9277, because - The status or check suite Linux_android android_platform_tests_shard_4 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 12, 2025
@chunhtai
Copy link
Contributor

ci is having issue now

@chunhtai chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 12, 2025
@auto-submit auto-submit bot merged commit 97fe921 into flutter:main Jun 12, 2025
78 checks passed
@StevenSorial StevenSorial deleted the routing-methods branch June 12, 2025 22:28
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 13, 2025
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Jun 13, 2025
flutter/packages@6d3aaf4...c5ab57a

2025-06-13 [email protected] [flutter_svg]
feat: Expose the renderStrategy property in SvgPicture
(flutter/packages#9373)
2025-06-12 [email protected] [go_router] Add routing functions to
GoRouteData (flutter/packages#9277)
2025-06-12 [email protected] [various] Update example apps to
Swift (flutter/packages#9347)
2025-06-12 [email protected] Roll Flutter from
824868f to f79452e (94 revisions) (flutter/packages#9419)
2025-06-12 [email protected]
[android_camera_camerax] Fix incorrect camera mirroring for front
cameras on devices using `ImageReader` Impeller backend
(flutter/packages#9233)

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-flutter-autoroll
Please CC [email protected] on the revert to ensure that a
human
is aware of the problem.

To file a bug in Flutter:
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
Ortes pushed a commit to Ortes/packages that referenced this pull request Jun 25, 2025
second step of fixing [#106790](flutter/flutter#106790). [First PR](flutter#9275)

This PR adds the routing methods `.location`, `.go(context)`, `.push(context)`, `.pushReplacement(context)`, and `replace(context)` to `GoRouteData`. They will be overridden by the mixin generated by `go_router_builder`

## Pre-Review Checklist

[^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
mboetger pushed a commit to mboetger/flutter that referenced this pull request Jul 21, 2025
…r#170600)

flutter/packages@6d3aaf4...c5ab57a

2025-06-13 [email protected] [flutter_svg]
feat: Expose the renderStrategy property in SvgPicture
(flutter/packages#9373)
2025-06-12 [email protected] [go_router] Add routing functions to
GoRouteData (flutter/packages#9277)
2025-06-12 [email protected] [various] Update example apps to
Swift (flutter/packages#9347)
2025-06-12 [email protected] Roll Flutter from
824868f to f79452e (94 revisions) (flutter/packages#9419)
2025-06-12 [email protected]
[android_camera_camerax] Fix incorrect camera mirroring for front
cameras on devices using `ImageReader` Impeller backend
(flutter/packages#9233)

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-flutter-autoroll
Please CC [email protected] on the revert to ensure that a
human
is aware of the problem.

To file a bug in Flutter:
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
vashworth pushed a commit to vashworth/packages that referenced this pull request Jul 30, 2025
second step of fixing [#106790](flutter/flutter#106790). [First PR](flutter#9275)

This PR adds the routing methods `.location`, `.go(context)`, `.push(context)`, `.pushReplacement(context)`, and `replace(context)` to `GoRouteData`. They will be overridden by the mixin generated by `go_router_builder`

## Pre-Review Checklist

[^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: go_router
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants