[go_router_builder] Add support for relative routes
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
- [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
- [x] I read the Tree Hygiene page, which explains my responsibilities.
- [x] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use
dart format.) - [x] I signed the CLA.
- [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g.
[shared_preferences] - [x] I linked to at least one issue that this PR fixes in the description above.
- [x] I updated
pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes. - [x] I updated
CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes. - [x] I updated/added relevant documentation (doc comments with
///). - [x] I added new tests to check the change I am making, or this PR is test-exempt.
- [x] All existing and new tests are passing.
If you need help, consider asking for advice on the #hackers-new channel on Discord.
@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?
Hi @ThangVuNguyenViet looks like there are some test failures, can you fix it so that we can land this pr?
sure I'll do that
@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?
Hi @ThangVuNguyenViet , do you have a chance to look into the error?
I'll check that out. Sorry for late reply, not seeing the email
@btrautmann hey could you provide reproducible code? I can't reproduce it
@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!
Idk, seems like there is an error, which I can't reproduce
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.
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
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
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?
I thinks it's more of a dev ex issue?
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?
Hey, weekly check in, can we merge this as soon as possible?
I also would love this.
Just did another merge conflict. Is there a better way to avoid conflict in the changelog file?
@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.
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?
@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?
cc @ThangVuNguyenViet for the pubspce and CI issues mentioned
Hi there @ThangVuNguyenViet, thanks much for contributing! Are you still able to work on this to complete the few things mentioned?
hey @mit-mit I don't think what she mentioned was relevant
Hey @ThangVuNguyenViet I'd love to help you get this in. Currently there are several failing tests, can you take a look?
@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
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
- That PR assumes there are only that set of changes in
GoRouteData, since at the time there was noTypedRelativeGoRoute. This PR introducesgoRelative,pushRelative, etc, which breaks the assumption. - The PR changes go_router_builder from generating
extensiontomixin, which is a breaking change. That list of methods added prevents compile time error when migrating to the go router version where it usesmixininstead ofextension. 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
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
@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
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.