packages icon indicating copy to clipboard operation
packages copied to clipboard

[go_router_builder] Add support for relative routes

Open ThangVuNguyenViet opened this issue 11 months ago • 22 comments

This PR is a 2nd part of #6825 to fully resolves https://github.com/flutter/flutter/issues/108177, which allow users to use TypedRelativeGoRoute to construct the route relatively.

This is a continuation of #7174

Example:

import 'package:go_router/go_router.dart';

const TypedRelativeGoRoute<RelativeRoute> relativeRoute =
    TypedRelativeGoRoute<RelativeRoute>(
  path: 'relative-route',
  routes: <TypedRoute<RouteData>>[
    TypedRelativeGoRoute<InnerRelativeRoute>(path: 'inner-relative-route')
  ],
);

@TypedGoRoute<Route1>(
  path: 'route-1',
  routes: <TypedRoute<RouteData>>[relativeRoute],
)
class Route1 extends GoRouteData {
  const Route1();
}

@TypedGoRoute<Route2>(
  path: 'route-2',
  routes: <TypedRoute<RouteData>>[relativeRoute],
)
class Route2 extends GoRouteData {
  const Route2();
}

class RelativeRoute extends GoRouteData {
  const RelativeRoute();
}

class InnerRelativeRoute extends GoRouteData {
  const InnerRelativeRoute();
}

Basically the added TypedRelativeGoRoute allows us to construct the route tree above. If we replace it with the existing TypedGoRoute it will declare multiple extensions of a same thing and produce build time error

Pre-launch Checklist

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

ThangVuNguyenViet avatar Jan 22 '25 08:01 ThangVuNguyenViet

@chunhtai same issue I asked in the old PR. The test will fail without the temp dependency_overrides. Should I make a PR that contains only the TypedRelativeGoRoute in go_router and wait for it to get merged first?

ThangVuNguyenViet avatar Jan 22 '25 08:01 ThangVuNguyenViet

Hi @ThangVuNguyenViet looks like there are some test failures, can you fix it so that we can land this pr?

chunhtai avatar Apr 24 '25 22:04 chunhtai

sure I'll do that

ThangVuNguyenViet avatar Apr 28 '25 18:04 ThangVuNguyenViet

@ThangVuNguyenViet I was doing a bit of testing of relative routes as we may be looking to use them in our project, and I had a question RE: the generation of location for a relative route. AFAICT, there are 2 ways to route using relative routes:

  • context.go(RelativeRoute().relativeLocation)
  • RelativeRoute().goRelative(context)

However, context.go(RelativeRoute().location) does not work as it fails the following assertion:

════════ Exception caught by foundation library ════════════════════════════════
The following assertion was thrown while dispatching notifications for GoRouteInformationProvider:
'package:go_router/src/match.dart': Failed assertion: line 235 pos 12: 'uri.path.startsWith(newMatchedLocation)': is not true.

When the exception was thrown, this was the stack:
#2      RouteMatchBase._matchByNavigatorKeyForGoRoute (package:go_router/src/match.dart:235:12)
match.dart:235
#3      RouteMatchBase._matchByNavigatorKey (package:go_router/src/match.dart:110:16)

Is there a use-case for using the location of a relative route? If not, should it even be generated?

btrautmann avatar Apr 29 '25 15:04 btrautmann

Hi @ThangVuNguyenViet , do you have a chance to look into the error?

chunhtai avatar May 08 '25 22:05 chunhtai

I'll check that out. Sorry for late reply, not seeing the email

ThangVuNguyenViet avatar May 16 '25 19:05 ThangVuNguyenViet

@btrautmann hey could you provide reproducible code? I can't reproduce it

ThangVuNguyenViet avatar May 17 '25 16:05 ThangVuNguyenViet

@ThangVuNguyenViet @chunhtai @hannah-hyj This looks awesome and offers lots of routing flexibility for apps at large scale. Looks like tests are passing. Could this be prioritized for merging? Thanks!

leofeng99 avatar May 20 '25 15:05 leofeng99

Idk, seems like there is an error, which I can't reproduce

thangmoxielabs avatar May 20 '25 15:05 thangmoxielabs

Idk, seems like there is an error, which I can't reproduce

@thangmoxielabs I think @btrautmann's comment on context.go(RelativeRoute().location) triggering an error is mostly a question around the .location API when used on a relative route. RelativeRoute().location returns a subpath of some full absolute path, therefore trying to go to it probably won't work. Example:

  • HomeRoute (/home)
    • RelativeRoute (/relative)

Doesn't work: context.go(RelativeRoute().location) -> context.go('/relative') Works: context.go('${HomeRoute().location}${RelativeRoute().location}') -> context.go('/home/relative')

It's more of a dev experience concern, and it doesn't seem merge blocking. One might find that having the .location method return a relative route is better so context.go(RelativeRoute().location) -> context.go('./relative'). But that sort of creates polymorphic behavior for the .location method which might causes other confusion.

leofeng99 avatar May 20 '25 19:05 leofeng99

Oh yea I just reread his comment. Thought he was saying the .relativeLocation doesn't work Well on my app I use the .location primarily to get the string, so I could use for sth like the nav bar selectedItem, e.g GoRouterState.of(context).matchedLocation.contains(RelativeRoute().location). I don't directly use it on context.go though But I'm going to create a PR which lets navigate to siblings route by "../", which that .location api will be used. Idk if I should add "goSibling" into that next PR or not

thangmoxielabs avatar May 20 '25 19:05 thangmoxielabs

And I do agree the relativeRoute.location is much much different from the normal route.location. Normal route is safe to call, no exception is expected. Relative routes are expected to throw exceptions if you don't know which route you're currently at. A trade off for the convenience

thangmoxielabs avatar May 20 '25 19:05 thangmoxielabs

We're hoping to use Relative Routes in an upcoming app version release (early July?) at our company. Is there any way we could accelerate merging this in? Happy to assist and contribute if that would help things move along-what are the blockers right now?

caseycrogers avatar May 29 '25 14:05 caseycrogers

I thinks it's more of a dev ex issue?

thangmoxielabs avatar May 29 '25 14:05 thangmoxielabs

Oh sorry I didn't mean that there was a blocker, I more just meant can we accelerate merging this? It looks like the only thing stopping us right now is you need to resolve the merge conflicts.

Once the conflicts are merged, we're just waiting on @chunhtai to merge right?

caseycrogers avatar May 29 '25 18:05 caseycrogers

Hey, weekly check in, can we merge this as soon as possible?

caseycrogers avatar Jun 05 '25 19:06 caseycrogers

I also would love this.

robertsonja avatar Jun 05 '25 21:06 robertsonja

Just did another merge conflict. Is there a better way to avoid conflict in the changelog file?

ThangVuNguyenViet avatar Jun 12 '25 06:06 ThangVuNguyenViet

@chunhtai is there any way you could step back in and help us get this merged? It's super high value for us and it looks like it has been done for a couple of months and just stuck in a merge conflict loop waiting for your approval and merge.

caseycrogers avatar Jun 18 '25 14:06 caseycrogers

I am working on an updated workflow for flutter/packages to remove the pain point of the changelog and version merge conflicts, hoping to get that out sooner rather than later.

I can help shepherd this in, but please do update the pubspec to match what you have indicated as the new version in the changelog, that seems to be have been omitted. CI is also failing, can you take a look?

Piinks avatar Jun 18 '25 19:06 Piinks

@hannah-hyj I noticed you mentioned in https://github.com/flutter/packages/pull/8665 that PRs should be split between go_router and go_router_builder. Is that not relevant here?

Piinks avatar Jun 18 '25 19:06 Piinks

cc @ThangVuNguyenViet for the pubspce and CI issues mentioned

caseycrogers avatar Jun 18 '25 19:06 caseycrogers

Hi there @ThangVuNguyenViet, thanks much for contributing! Are you still able to work on this to complete the few things mentioned?

mit-mit avatar Jul 01 '25 15:07 mit-mit

hey @mit-mit I don't think what she mentioned was relevant

ThangVuNguyenViet avatar Jul 01 '25 15:07 ThangVuNguyenViet

Hey @ThangVuNguyenViet I'd love to help you get this in. Currently there are several failing tests, can you take a look?

Piinks avatar Jul 08 '25 17:07 Piinks

@Piinks there seems to be a new convention for go_router_builder, which requires with _$RouteMixin. I'll try to update and fix the issue

ThangVuNguyenViet avatar Jul 09 '25 04:07 ThangVuNguyenViet

There are lots of breaking changes in #9277 that results in this test run failure. Specifically it throws Should be generated using [Type-safe routing](https://pub.dev/documentation/go_router/latest/topics/Type-safe%20routes-topic.html).

After carefully inspected the changes, I've removed 1 breaking change that that PR made, which is the adding of routing methods \.location, \.go\(context\), \.push\(context\), \.pushReplacement\(context\), and replace\(context\) to GoRouteData. I find them should be removed because

  1. That PR assumes there are only that set of changes in GoRouteData, since at the time there was no TypedRelativeGoRoute. This PR introduces goRelative, pushRelative, etc, which breaks the assumption.
  2. The PR changes go_router_builder from generating extension to mixin, which is a breaking change. That list of methods added prevents compile time error when migrating to the go router version where it uses mixin instead of extension. That's why I wasn't able to detect error until tests run.

I'm removing that set of routing methods in the upcoming commits, and adapting the generation of TypedRelativeGoRoute from extension to mixin as well. Please let me know what I should do instead

ThangVuNguyenViet avatar Jul 09 '25 06:07 ThangVuNguyenViet

btw I am unable to run the ensure_build test on my local. The error message says Could not find a command named "/Users/vietthangvunguyen/Workspace/flutter/bin/cache/dart-sdk/bin/snapshots/frontend_server.dart.snapshot". do you have any idea? The test still runs fine on CI

ThangVuNguyenViet avatar Jul 09 '25 07:07 ThangVuNguyenViet

@Piinks please assist me on the breaking change. I think removing that part was right, but if you tell me that wasn't the right direction, I'm happy to bring it back and make changes as needed

ThangVuNguyenViet avatar Jul 09 '25 15:07 ThangVuNguyenViet

I've removed 1 breaking change that that PR made

I don't think undoing the other change as part of this one is the right course of action. That change has already been published, and undoing it would be another breaking change.

Piinks avatar Jul 09 '25 15:07 Piinks