[go_router] Refactor internal classes and methods

[johnpryan]

This moves some things around to make things easier to change. The GoRouteInformationParser now returns a RouteMatchList object by delegating the matching to a RouteMatcher object. Once the initial matches are found, the parser processes redirects using an abstract RouteRedirector function. After redirecting, the parsing is done and the RouteMatchList is provided to GoRouterDelegate through setNewRoutePath, where it stores the current RouteMatchList.

Once the GoRouterDelegate updates its RouteMatchList state, it builds the widget tree by delegating to RouteBuilder. RouteBuilder is a new class that builds widget tree starting with the top-level Navigator all the way down to the active sub-route.

List of changes:

• Separate matching from redirection
• Add RouteRedirector typedef
• Add RouteMatcher class
• Add RouteBuilder class
• Add RouteConfiguration class
• Rename and reorganize internal classes and libraries
• Add todo comments

I didn't modify the tests too much, but this should make it possible to write more specific tests for matching, redirection, building, etc.

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.

[loic-sharma]

Suggested change

/// Returns true if the redirect was processed.

[loic-sharma]

What do you think of moving comments to before the argument? Same comment for line 130.

Suggested change

	name: null,
	// no name available at the top level
	// no name available at the top level
	name: null,

[loic-sharma]

Just to double-check, is super.toString() guaranteed to have some sort of trailing whitespace?

[loic-sharma]

LGTM but please make sure to check API docs for unexpected breaking changes before merging!

[johnpryan]

I diffed the API docs this branch and main, and found that we were only exporting two typedefs, so I changed that back: 7f8954c. Other than that, I don't think this will be a breaking change.

Technically, we are also exporting routeConfiguration on GoRouter, but it is marked @visibleForTesting so I don't consider this a reason to bump the minor version

[chunhtai]

I like the direction this pr is going, but I am a bit concern about some of the class renaming and this can be a breaking change for developer who use the goroute- delegates directly.

[chunhtai]

The properties in this class are only needed in gorouteinformationparser once we remove the namedLocation. I am a bit unsure whether we should introduce a new class that will probably be deprecated in the future

[johnpryan]

The routeConfiguration field is marked @visibleForTesting in RouterDelegate, so I don't consider this a new public API. The reason for making this a separate class is make it easier to change the configuration without updating all of the places where the routeConfiguration is being passed around. For example, we might want to deprecate topRedirect.

[chunhtai]

but at somepoint we won't need to pass it around.

We would need to expose this class if we expose the GoRouterDelegate, otherwise, developer won't be able to use GoRouterDelegate constructor directly.

[johnpryan]

Filed an issue to remove this along with the namedLocation API: flutter/flutter#108139

[chunhtai]

I feel we will still need to expose the GoRoute- delegates if customers want to create their own delegates to use with the GoRouter- delegates

[johnpryan]

This class isn't currently part of the public API, but yes if we wanted to make it configurable as the part of the constructor we would have to export it:

import 'package:go_router/go_router.dart'
class CustomMatchListParser extends MatchListParser {
  // ...
}

// ... 
  final _router = GoRouter(
    builder: CustomMatchListBuilder(),
    parser: CustomMatchListParser(),
    matcher: CustomMatchListMatcher(),
    redirector: CustomMatchListRedirector(),
    routes: [
      // ...
    ],

We could take it one step further and allow users to specify a custom implementation of RouteConfiguration.

[chunhtai]

I mean unless we are never going to expose GoRouteInfromationParser/GoRouterDelegate, it doesn't make sense to change the type back to RouteInformationParser given that it is already exposed as GoRouteInformationParser. I think right now this package is in a weird state that some classes should have been exposed as public but don't, and it was too late to making it completely private because some other classes have exposed them as part of their APIs

[johnpryan]

I think I misunderstood your question. There was a discussion about this in a previous review: #2317 (comment)

[chunhtai]

LGTM

[chunhtai]

Maybe link this ? flutter/flutter#107729

[chunhtai]

same flutter/flutter#107729