Skip to content

[go_router]Fixes GoRouterState.location and GoRouterState.param to return correct value #2786

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Nov 21, 2022
Merged

[go_router]Fixes GoRouterState.location and GoRouterState.param to return correct value #2786

merged 3 commits into from
Nov 21, 2022

Conversation

chunhtai
Copy link
Contributor

@chunhtai chunhtai commented Nov 7, 2022 •
edited
Loading

fixes flutter/flutter#112444
fixes flutter/flutter#111040

also cleans up RouteMatch and RouteMatchlist. This pr move all the properties in RouteMatch class that are the same for all of RouteMatch to RouteMatchlist to avoid duplicating data too many time.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • 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.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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

@chunhtai chunhtai changed the title [go_router]Fixes GoRouterState.locatio and GoRouterState.param to return correct value [go_router]Fixes GoRouterState.location and GoRouterState.param to return correct value Nov 7, 2022
@cgestes
Copy link

cgestes commented Nov 8, 2022

Does it also fixes wrong GoRouterState (accessed using GoRouteState.of(context)) when using router.push ?

@chunhtai
Copy link
Contributor Author

chunhtai commented Nov 8, 2022

Does it also fixes wrong GoRouterState (accessed using GoRouteState.of(context)) when using router.push ?

I am not aware of the issue, is there an existing github issue? if not, can you file a new issue for this?

@cgestes
Copy link

cgestes commented Nov 8, 2022

Ok. let me try to reproduce on a simple test case.

@cgestes
Copy link

cgestes commented Nov 8, 2022

here we go: flutter/flutter#114901

chunhtai
chunhtai commented Nov 15, 2022
View reviewed changes
packages/go_router/lib/src/configuration.dart
@@ -20,74 +20,95 @@ class RouteConfiguration {
required this.redirectLimit,
required this.topRedirect,
required this.navigatorKey,
}) {
}) : assert(_debugCheckPath(routes, true)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

all RouteBase integrity check will be here, instead of separated in different constructor

chunhtai
chunhtai commented Nov 15, 2022
View reviewed changes
packages/go_router/lib/src/delegate.dart

/// The route match that represent route pushed through [GoRouter.push].
// TODO(chunhtai): Removes this once imperative API no longer insert route match.
class ImperativeRouteMatch extends RouteMatch {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a workaround that should be removed once the imperative API refactor is done

packages/go_router/lib/src/matching.dart
// If there are multiple routes that match the location, returning the first one.
// To make predefined routes to take precedence over dynamic routes eg. '/:id'
// consider adding the dynamic route at the end of the routes
return result.first;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This always returns first, might as well use a single object to hold the result. This is probably some left over code from previous refactoring.

@chunhtai chunhtai force-pushed the issues/112444 branch 2 times, most recently from eab4a9d to 08ca9cb Compare November 15, 2022 19:43
@chunhtai chunhtai requested a review from johnpryan November 15, 2022 19:44
@ahmednfwela
Copy link
Contributor

ahmednfwela commented Nov 17, 2022 •
edited
Loading

@chunhtai so now to access the latest values via GoRouter we do
GoRouter.routerDelegate.currentConfiguration.pathParameters, GoRouter.routerDelegate.currentConfiguration.uri.queryParameters, GoRouter.routerDelegate.currentConfiguration.uri.toString() is that right ?

@chunhtai
Copy link
Contributor Author

GoRouter

you can do GoRouterState.of(context).params GoRouterState.of(context).queryParams GoRouterState.of(context).location

johnpryan
johnpryan reviewed Nov 17, 2022
View reviewed changes
@johnpryan
Copy link
Contributor

@chunhtai I fixed the merge conflicts with master, but a few of the replace() tests are failing. Could you take a look at those?

ahmednfwela
ahmednfwela reviewed Nov 21, 2022
View reviewed changes
packages/go_router/lib/src/builder.dart Outdated
@@ -77,7 +78,7 @@ class RouteBuilder {
return _buildErrorNavigator(
context,
e,
Uri.parse(matchList.location.toString()),
Uri.parse(matchList.uri.toString()),
Copy link
Contributor

Choose a reason for hiding this comment

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

can't this entire line be just matchList.uri ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch

@chunhtai chunhtai requested a review from johnpryan November 21, 2022 17:56
@chunhtai chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 21, 2022
@auto-submit auto-submit bot merged commit 92c60ee into flutter:main Nov 21, 2022
@ahmednfwela
Copy link
Contributor

@chunhtai are there any plans to expose RouteMatchList into the public API now that it's cleaned up ?

@chunhtai
Copy link
Contributor Author

@ahmednfwela yes, that will be my next step. I will open up most of the internal classes.

This was referenced Nov 23, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahmednfwela ahmednfwela ahmednfwela left review comments

@johnpryan johnpryan johnpryan approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: go_router
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[go_router] ShellRoute doesn't correctly parse GoRouterState.params [go_router] GoRouterState regression
4 participants
@chunhtai @cgestes @ahmednfwela @johnpryan