[draft] [go_router] wrap path_utils content in RoutePattern #7706
Conversation
| assert(pathParameters.containsKey(paramName), | ||
| 'missing param "$paramName" for $path'); | ||
| 'missing param "$paramName" for ${route.pattern}'); |
There was a problem hiding this comment.
( RoutePattern overrides toString )
There was a problem hiding this comment.
this may be a breaking change, this could be an easy migration if you write rules fix_data.yaml
Also in this library, path means /book/:bookId, the filled string like book/123 is called location.
cedvdb
changed the title
| /// | ||
| /// Builds the absolute path for the route, by concatenating the paths of the | ||
| /// route and all its ancestors. | ||
| String? locationForRoute(RouteBase route) => |
There was a problem hiding this comment.
I'll readd this method later as it is a Public API, but for the time being, the signature has changed for the internals.
It doesn't seem like this should be part of the Public API though ?
|
|
||
| /// The path parameters in this route. | ||
| @internal | ||
| final List<String> pathParameters = <String>[]; | ||
| List<String> get pathParameters => pattern.parameters; |
There was a problem hiding this comment.
Not sure if the pattern should be accessed directly (route.pattern.parameters) or if the route should facade to the pattern.
There was a problem hiding this comment.
if we expose the pattern, than they should probably be accessed directly to avoid duplication
There was a problem hiding this comment.
do we want to expose pattern though?
There was a problem hiding this comment.
Currently no, the idea was to have RoutePattern used internally, (I have to make it private or internal in the future) and have the public representation still String path, ie: GoRoute(path: '/some-path'). The RoutePattern is supposed to be used internally everywhere so we don't end up with a String path, where we are unsure whether path is /books/:bookId or /books/123 without looking at the wider context. Although I'm not a fan of this kind of constructor conversion, in this case I think it's worth it.
However, the method toPath should be public GoRoute().toPath because it's useful when building a redirect link for a third party and it's currently not exposed I think (from path_utils). This brings ambiguosity though, because there is already GoRoute('').path. I may have to rename RoutePattern into RoutePath, and toPath() into reverse(), it may be less ambiguous that way.
There was a problem hiding this comment.
in that case the RoutePattern should be kept in as private variable in GoRoute class.
| assert(pathParameters.containsKey(paramName), | ||
| 'missing param "$paramName" for $path'); | ||
| 'missing param "$paramName" for ${route.pattern}'); |
There was a problem hiding this comment.
this may be a breaking change, this could be an easy migration if you write rules fix_data.yaml
Also in this library, path means /book/:bookId, the filled string like book/123 is called location.
|
|
||
| /// The path parameters in this route. | ||
| @internal | ||
| final List<String> pathParameters = <String>[]; | ||
| List<String> get pathParameters => pattern.parameters; |
There was a problem hiding this comment.
if we expose the pattern, than they should probably be accessed directly to avoid duplication
|
|
||
| /// The path parameters in this route. | ||
| @internal | ||
| final List<String> pathParameters = <String>[]; | ||
| List<String> get pathParameters => pattern.parameters; |
There was a problem hiding this comment.
do we want to expose pattern though?
I believe it's possible to make it only internal without breaking changes, that's the requirement I'm doing this under |
|
if we want to rename path -> pattern I would like us to change the public facing API such as but i am not sure if this worth it. As for wrapping path_util, I think we can do it as less as breaking change as possible, and I am onboard with this. |
|
@chunhtai So the convention used is that |
yes. that is the current naming convention. |
|
(triage) Hi @cedvdb are you still planning on coming back to this pr? |
|
@chunhtai This is not my priority, so I don't think I will in the near future at least. So you can close, I'll reopen if I happen to come back to this |
|
Thanks for reply, I will close this pr for now, feel free to reopen if you have time in the future |
part of flutter/flutter#155567
This is a draft PR to see if some part of go_router can be refactored to be more readable.
Currently it is hard to know if a parameter / variable is a pattern such as
/books/:bookIdor a path such as/book/123when reading the code of a given function. This PR is a stab on improving the readability on that front.It wraps the content of path_utils in a
RoutePatternclass.@chunhtai this does not compile but I'm interested in knowing whether this is a direction the team wants to go or not, I'll just close this if not.
I'll probably wait for my other PR's to go through before going any further anyway.
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.