Skip to content

[go_router_builder] Add support for relative routes #8476

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

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

ThangVuNguyenViet
Copy link
Contributor

@ThangVuNguyenViet ThangVuNguyenViet commented Jan 22, 2025

This PR is a 2nd part of #6825 to fully resolves flutter/flutter#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
Copy link
Contributor Author

@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?

final GoRouter _router = GoRouter(
routes: $appRoutes,
);
const TypedRelativeGoRoute<DetailsRoute> detailRoute =
Copy link
Contributor

Choose a reason for hiding this comment

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

What is different between using this vs a regular TypedGoRoute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The example I gave isn't clearly explaining the purpose. Let me update this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Contributor

Choose a reason for hiding this comment

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

I still can't see the difference. Is this a way to reuse the typedGoRoute in multiple places in the routing tree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Without this you'd have to define routes like
SettingsFromHomeRoute
SettingsFromDashboardRoute

And those routes all build (or build page) the same screen

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah this makes sense. I will take another look

Copy link
Contributor Author

@ThangVuNguyenViet ThangVuNguyenViet left a comment

Choose a reason for hiding this comment

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

just updated the example

final GoRouter _router = GoRouter(
routes: $appRoutes,
);
const TypedRelativeGoRoute<DetailsRoute> detailRoute =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@chunhtai chunhtai self-requested a review February 6, 2025 23:32
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.

See my comment #8476 (comment)

@chunhtai chunhtai self-requested a review February 27, 2025 23:05
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

@chunhtai chunhtai requested a review from hannah-hyj March 28, 2025 20:01
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
Copy link
Contributor

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

@ThangVuNguyenViet
Copy link
Contributor Author

sure I'll do that

@btrautmann
Copy link

@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?

@chunhtai
Copy link
Contributor

chunhtai commented May 8, 2025

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

@ThangVuNguyenViet
Copy link
Contributor Author

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

@ThangVuNguyenViet
Copy link
Contributor Author

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

@leofeng99
Copy link

@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!

@thangmoxielabs
Copy link

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

@leofeng99
Copy link

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.

@thangmoxielabs
Copy link

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
Copy link

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

@caseycrogers
Copy link

caseycrogers commented May 29, 2025

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?

@thangmoxielabs
Copy link

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

@caseycrogers
Copy link

caseycrogers commented May 29, 2025

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[go_router] Add support for relative routes
7 participants