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] Add support for relative routes #6825

Merged
merged 20 commits into from
Nov 13, 2024

Conversation

ThangVuNguyenViet
Copy link
Contributor

@ThangVuNguyenViet ThangVuNguyenViet commented May 29, 2024

Add supports for relative routes by allowing going to a path relatively, like go('./$path')

This PR doesn't fully resolve any issue, but it's mandatory to further add examples & tests for TypedRelativeGoRoute (see #7174), which will resolves #108177

Pre-launch Checklist

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

@ThangVuNguyenViet
Copy link
Contributor Author

ThangVuNguyenViet commented May 31, 2024

@chunhtai I separated the PR as suggested. Please let me know the next steps
It was my project's need for the goRelative that I created this PR, but it's also tempted to pushRelative along with other imperative APIs. However, that'll result in a new set of APIs (goRelative, pushRelative, pushReplacementRelative , etc). Alternatively we can do make a helper function that helps user construct the relativeRoute, like context.go(context.relativeRoute('settings'). Wdyt?

@cedvdb
Copy link
Contributor

cedvdb commented Jun 4, 2024

Why don't you add it to the existing api.

context.go(`./settings`)

is pretty common in router modules in other frameworks as well as in bash

@ThangVuNguyenViet
Copy link
Contributor Author

Why don't you add it to the existing api.


context.go(`./settings`)

is pretty common in router modules in other frameworks as well as in bash

Oh yea that's cool. I didn't know of that pattern

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.

I also agree we should probably use go(./something) for relative path instead of adding new API. Seems more straigtforward

);

final Uri currentUri = value.uri;
Uri newUri = Uri.parse(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just modify the path and use currentUri.replace(path: newPath)
This way you don't need to worry about other thing such as host/scheme/fragment/queryparameters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha. I move this in path_utils as concatenateUris

@ThangVuNguyenViet ThangVuNguyenViet force-pushed the go_router/go-relative branch 2 times, most recently from 9aa61bd to 975128b Compare June 19, 2024 13:05
@chunhtai chunhtai self-requested a review June 20, 2024 21:39
/// Concatenates two Uri. It will [concatenatePaths] the parent's and the child's paths , then merges their query parameters.
///
/// e.g: pathA = /a?fid=f1, pathB = c/d?pid=p2, concatenatePaths(pathA, pathB) = /a/c/d?fid=1&pid=2.
Uri concatenateUris(Uri parentUri, Uri childUri) {
Copy link
Contributor

Choose a reason for hiding this comment

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

the path looks good, but I am a bit hesitate on also merge query parameter. It has different logic from regular go. Regular go will not attempt to merge with current query parameters. I think we should probably follow the same logic to avoid confusion.

Choose a reason for hiding this comment

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

Hmm this is quite interesting thing thou. Take this example.

We're in /family/f2?sort=desc and we're going to the inner person/p1 route, and we'll pop it to go back to the family route. How are devs handling this case?

In go_router_builder, devs can't use the easy PersonRoute(pid: 'p1').go but rather have to get its route.location, convert to Uri, and then manually add the sort param. Being able to use Route().go in builder is kinda a big selling point of the package imo

Copy link
Contributor

Choose a reason for hiding this comment

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

you can access GoRouterState.of(context).uri.queryParemeters.

Also won't in this case PersonRoute also declare sort parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make sense. I've been doing things wrong in my app then

@@ -390,6 +390,28 @@ class TypedGoRoute<T extends GoRouteData> extends TypedRoute<T> {
final List<TypedRoute<RouteData>> routes;
}

/// A superclass for each typed go route descendant
@Target(<TargetKind>{TargetKind.library, TargetKind.classType})
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also create a go_router_builder pr that shows how this will be used?

Choose a reason for hiding this comment

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

gotcha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's #7174 @chunhtai , and I also updated the PR desc to reflect that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chunhtai I've removed this class as discussed in the #7174

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.

just pointing out one issue, otherwise, LGTM

Uri concatenateUris(Uri parentUri, Uri childUri) {
Uri newUri = parentUri.replace(
path: concatenatePaths(parentUri.path, childUri.path),
queryParameters: childUri.queryParameters,
Copy link
Contributor

Choose a reason for hiding this comment

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

also fragment

Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably have some test for fragment as well

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

@cedvdb
Copy link
Contributor

cedvdb commented Oct 20, 2024

@thangmoxielabs This branch has conflicts that must be resolved

@vasilich6107
Copy link
Contributor

vasilich6107 commented Nov 2, 2024

@ThangVuNguyenViet This branch has conflicts that must be resolved
@chunhtai @hannah-hyj is there any chance to fix conflicts and merge this very important feature?(How can I help?)

@cedvdb
Copy link
Contributor

cedvdb commented Nov 2, 2024

@ThangVuNguyenViet This branch has conflicts that must be resolved @chunhtai @hannah-hyj is there any chance to fix conflicts and merge this very important feature?(How can I help?)

You can fork and redo a pr

@thangmoxielabs
Copy link

@cedvdb hey sorry for the late reply. I didn't see any notification

@vasilich6107
Copy link
Contributor

@chunhtai @hannah-hyj
Could you merge this PR and release

@chunhtai
Copy link
Contributor

chunhtai commented Nov 7, 2024

This somehow slip through the crack. @thangmoxielabs can you fix the conflict again? sorry about the delay

@ThangVuNguyenViet
Copy link
Contributor Author

@chunhtai resolved it

@chunhtai
Copy link
Contributor

chunhtai commented Nov 7, 2024

Hi @ThangVuNguyenViet you also need to bump the package.yaml version

@ThangVuNguyenViet
Copy link
Contributor Author

Hey @chunhtai it's bumped

@vasilich6107
Copy link
Contributor

vasilich6107 commented Nov 12, 2024

Hi @chunhtai
go_router was again updated 5 hours ago
Is there any chance to merge this PR before all other?

@chunhtai chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 13, 2024
@auto-submit auto-submit bot merged commit 67d8b50 into flutter:main Nov 13, 2024
77 checks passed
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 13, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 13, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 13, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 13, 2024
flutter/packages@72356fd...26e123a

2024-11-13 dasyad00@gmail.com [camera_windows] Set device media type for video preview explicitly (flutter/packages#7447)
2024-11-13 39979207+ThangVuNguyenViet@users.noreply.github.com [go_router] Add support for relative routes (flutter/packages#6825)
2024-11-13 pq@users.noreply.github.com [vector_graphics_compiler] fix a renamed method parameter lint (flutter/packages#8070)
2024-11-12 feinstein@users.noreply.github.com [in_app_purchase] Add expiration date to Transaction (flutter/packages#8030)
2024-11-12 stuartmorgan@google.com [various] Clean up contributing guides (flutter/packages#8032)
2024-11-12 ditman@gmail.com [ci] Remove web renderer option from tools. (flutter/packages#8055)
2024-11-12 stuartmorgan@google.com [url_launcher] Update Pigeon version for Linux (flutter/packages#8065)
2024-11-12 tobias@leafnode.se [go_router] Add support for preloading branches of StatefulShellRoute (revised solution) (flutter/packages#6467)
2024-11-12 stuartmorgan@google.com [pigeon] Make Linux type declarations public (flutter/packages#8040)
2024-11-11 engine-flutter-autoroll@skia.org Roll Flutter from 73546b3 to c8510f2 (30 revisions) (flutter/packages#8042)
2024-11-11 magder@google.com Use dependabot multi-directory configuration for Android package updates (flutter/packages#8048)
2024-11-11 stuartmorgan@google.com [tools] Run `pub get` before `format` (flutter/packages#8052)
2024-11-11 stuartmorgan@google.com [file_selector] Fix Linux cancel regression (flutter/packages#8051)
2024-11-09 stuartmorgan@google.com [shared_preferences] Fix confusing language in README (flutter/packages#8049)
2024-11-08 magder@google.com Use dependabot multi-directory configuration for Android example gradle updates (flutter/packages#8036)
2024-11-08 43054281+camsim99@users.noreply.github.com [animations] Remove `.flutter-plugins` reference from example app (flutter/packages#8002)
2024-11-08 magder@google.com Group dependabot github-action update PRs, delete dead docker updates (flutter/packages#8044)
2024-11-08 37028599+EArminjon@users.noreply.github.com [vector_graphics_compiler] fix-null-exception (flutter/packages#8006)
2024-11-08 stuartmorgan@google.com [tools] Format Dart per-package (flutter/packages#8043)

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 flutter-ecosystem@google.com 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
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
6 participants