-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[go_router_builder] Add go_router StatefulShellRoute support to go_router_builder #4238
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
Conversation
This reverts commit 68f99cb4ac66ca445b37bdc8619875f163b739f3.
855cce7 to
bc19f53
Compare
issue: [#127371](flutter/flutter#127371) go_router_builder_pr #4238
| required this.navigatorKey, | ||
| required super.routeDataClass, | ||
| required super.parent, | ||
| required super.parentNavigatorKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StatefulShellBranch doesn't support parent navigator key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have another more generic base class for RouteBaseConfig so that it can be extended by StatefulShellBranchConfig without the unused logic or property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm, do you mean like
RouteOrBranchBaseConfig -> RouteBaseConfig -> (GoRouteConfig, StatefulShellRouteConfig, etc)
RouteOrBranchBaseConfig -> StatefulShellBranchConfig
|
|
||
| @override | ||
| String get routeConstructorParameters => | ||
| navigatorKey == null ? '' : 'navigatorKey: $navigatorKey,'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no navigator key, but we need to have a way to pass in
navigatorContainerBuilder and restorationScopeId
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| ), | ||
| ); | ||
| break; | ||
| default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use case 'TypedGoRoute':?
it should throw if it is anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| value._children.addAll(reader.read('routes').listValue.map<RouteBaseConfig>( | ||
| (DartObject e) => RouteBaseConfig._fromAnnotation( | ||
| value._children.addAll(reader | ||
| .read(typeName == 'TypedStatefulShellRoute' ? 'branches' : 'routes') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be a getter in the RouteBaseConfig? In general we should try to have less check like this in this method as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| ? '' | ||
| : ''' | ||
| routes: [${_children.map((RouteBaseConfig e) => '${e._invokesRouteConstructor()},').join()}], | ||
| ${routeDataClassName == 'StatefulShellRouteData' ? 'branches' : 'routes'}: [${_children.map((RouteBaseConfig e) => '${e._invokesRouteConstructor()},').join()}], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| ? '' | ||
| : 'parentNavigatorKey: $parentNavigatorKey,'; | ||
|
|
||
| if (routeDataClassName == 'StatefulShellBranchData') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| class StatefulShellRouteConfig extends RouteBaseConfig { | ||
| StatefulShellRouteConfig._({ | ||
| required super.routeDataClass, | ||
| required super.parent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parent navigator key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| classElement, | ||
| parameterName: r'$parentNavigatorKey', | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a default and throws? it will be useful to remind people when they add a new typed route
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| /// The parent navigator key string that is used for initialize the | ||
| /// `RouteBase` class this config generates. | ||
| final String? parentNavigatorKey; | ||
| static String _generateChildrenGetterName(String name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably exposed as a getter for subclass to override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's used in the factory function factory RouteBaseConfig._fromAnnotation and Instance members can’t be accessed from a factory constructor.
chunhtai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice work!
|
auto label is removed for flutter/packages, pr: 4238, due to Pull request flutter/packages/4238 is not in a mergeable state. |
…to go_router_builder (flutter/packages#4238)
…to go_router_builder (flutter/packages#4238)
…to go_router_builder (flutter/packages#4238)
flutter/packages@4e4961a...d00c1f9 2023-08-03 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.21.1 to 2.21.2 (flutter/packages#4637) 2023-08-03 engine-flutter-autoroll@skia.org Roll Flutter from b3f99ff to c00d241 (12 revisions) (flutter/packages#4640) 2023-08-03 jpnurmi@gmail.com [path_provider] Add getApplicationCachePath() - implementations (flutter/packages#4619) 2023-08-03 109253501+pdblasi-google@users.noreply.github.com [various] Removed references to deprecated `TestWindow` APIs (flutter/packages#4558) 2023-08-02 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 4.1.0 to 4.3.0 (flutter/packages#4432) 2023-08-02 jhy03261997@gmail.com [go_router_builder] Add go_router StatefulShellRoute support to go_router_builder (flutter/packages#4238) 2023-08-02 reidbaker@google.com [Tooling] Add google owned cache for dependencies as an option in ci (flutter/packages#4567) 2023-08-02 engine-flutter-autoroll@skia.org Roll Flutter from 1d59196 to b3f99ff (32 revisions) (flutter/packages#4634) 2023-08-02 32538273+ValentinVignal@users.noreply.github.com [go_router_builder] Support `ShellRouteData` without `const` constructor (flutter/packages#4627) 2023-08-02 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.21.0 to 2.21.1 (flutter/packages#4573) 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,rmistry@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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
fixes: flutter/flutter#127371