Skip to content

Conversation

@hannah-hyj
Copy link
Member

@hannah-hyj hannah-hyj commented Jun 16, 2023

This reverts commit 68f99cb4ac66ca445b37bdc8619875f163b739f3.
auto-submit bot pushed a commit that referenced this pull request Jul 7, 2023
@hannah-hyj hannah-hyj changed the title [draft][go_router_builder] Add go_router StatefulShellRoute support to go_router_builder [go_router_builder] Add go_router StatefulShellRoute support to go_router_builder Jul 10, 2023
@hannah-hyj hannah-hyj marked this pull request as ready for review July 10, 2023 01:03
@hannah-hyj hannah-hyj requested a review from chunhtai as a code owner July 10, 2023 01:03
required this.navigatorKey,
required super.routeDataClass,
required super.parent,
required super.parentNavigatorKey,
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Member Author

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,';
Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

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

done

),
);
break;
default:
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 use case 'TypedGoRoute':?
it should throw if it is anything else.

Copy link
Member Author

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')
Copy link
Contributor

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.

Copy link
Member Author

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()}],
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Member Author

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') {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Member Author

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,
Copy link
Contributor

Choose a reason for hiding this comment

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

parent navigator key?

Copy link
Member Author

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',
),
);
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 add a default and throws? it will be useful to remind people when they add a new typed route

Copy link
Member Author

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) {
Copy link
Contributor

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.

Copy link
Member Author

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.

@hannah-hyj hannah-hyj requested a review from chunhtai July 31, 2023 21:40
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, nice work!

@hannah-hyj hannah-hyj added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 1, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 2, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 2, 2023

auto label is removed for flutter/packages, pr: 4238, due to Pull request flutter/packages/4238 is not in a mergeable state.

@hannah-hyj hannah-hyj added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 2, 2023
@auto-submit auto-submit bot merged commit c3a5fb9 into flutter:main Aug 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 3, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 3, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 3, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 3, 2023
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
koji-1009

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App p: go_router_builder

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[go_router_builder] Add go_router StatefulShellRoute support to go_router_builder

3 participants