[go_router] Refactor RouterDelegate into functional pieces #1653

chunhtai · 2022-04-27T20:49:18Z

This change will break the following:

customer will need to also pass in the goRouter.routeInfromationProvider.
GoRouterDelegate.namedLocation is move to GoRouteInfromationParser.namedLocation

breaking change doc flutter.dev/go/go-router-v4-breaking-changes

Pre-launch Checklist

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

johnpryan

LGTM

johnpryan · 2022-05-23T14:34:52Z

packages/go_router/lib/src/go_route_information_provider.dart

nit: I think you can use this type directly as long as we set the Dart SDK constraint to >=2.17.0 <3.0.0

Suggested change

// ignore: unnecessary_non_null_assertion

static WidgetsBinding get _binding => WidgetsBinding.instance!;

static WidgetsBinding get _binding => WidgetsBinding.instance;

I think the cirrus still use 2.16.xx, I can skip it but I think it is probably better to keep this so that it can still be analyzed

johnpryan · 2022-05-23T14:35:50Z

packages/go_router/lib/src/go_route_information_provider.dart

Why does selectMultiEntryHistory() need to be called? The docs say it's the default.

If there is another MateriaApp wraps on top, it may have switched to single entry history.

johnpryan · 2022-05-23T14:46:54Z

packages/go_router/lib/src/go_route_information_provider.dart

nit: add a comment:

Suggested change

final bool replace = type == RouteInformationReportingType.neglect ||

// Avoid adding a new history entry if the route is the same as before.

final bool replace = type == RouteInformationReportingType.neglect ||

johnpryan · 2022-05-23T14:55:02Z

packages/go_router/lib/src/go_route_match.dart

Was this removed because it was unused? I see that namedLocation moved from GoRouterDelegate into GoRouteInformationParser, but I don't see what happened to this method.

yes i moved the functionality to goRouteInformationParser, GoRouteMatch is not exposed to the package level

johnpryan · 2022-05-23T14:56:07Z

packages/go_router/lib/src/go_router_state.dart

johnpryan · 2022-05-23T15:16:00Z

packages/go_router/lib/src/go_route_information_parser.dart

It doesn't look like there are any tests for this method - I'll probably add some when I start working on flutter/flutter#99126

This method should be covered by a lot of test indirectly, any test that build the page from uri should do

Right, but I think it's important enough to have it's own set of tests eventually.

I can add it in this patch

Not sure if it's helpful, but there's some test cases in this file that might be useful to consider: https://github.com/johnpryan/tree_router/blob/main/test/tree_test.dart

packages/go_router/lib/src/go_router.dart

johnpryan · 2022-05-26T19:24:25Z

It looks like there's still some analyzer warnings:

Running command: "dart analyze --fatal-infos" in /tmp/cirrus-ci-build/packages/go_router
Analyzing go_router...

   info - lib/src/go_route_information_parser.dart:374:25 - Separate the control structure expression from its statement. - always_put_control_body_on_new_line
   info - test/go_route_information_parser_test.dart:141:42 - This function has a nullable return type of 'String?', but ends without returning a value. Try adding a return statement, or if no value is ever returned, try changing the return type to 'void'. - body_might_complete_normally_nullable
   info - test/go_route_information_parser_test.dart:142:50 - Separate the control structure expression from its statement. - always_put_control_body_on_new_line

…1653)" This reverts commit 2c02052.

…1653)" (#2183) This reverts commit 2c02052.

stuartmorgan-g · 2022-06-01T19:36:55Z

Please make sure flutter/flutter#105150 (comment) is addressed before this is re-landed in any form (or any other changes are landed to go_router, ideally).

johnpryan · 2022-06-01T19:48:50Z

Would it be acceptable to combine this with #2177? If I understand correctly, as long as both packages are touched in go_router and go_router_builder, everything will be tested properly in presubmit.

cc: @kevmoo

johnpryan · 2022-06-01T19:49:27Z

It sounds like the better option is to combine go_router_builder and go_router into a single package?

stuartmorgan-g · 2022-06-01T19:51:43Z

Would it be acceptable to combine this with #2177? If I understand correctly, as long as both packages are touched in go_router and go_router_builder, everything will be tested properly in presubmit.

That would fix the problem in this particular instance, but leave the fundamental problem that makes changing go_router unsafe unaddressed. I would much rather we not leave landmines in the CI.

It sounds like the better option is to combine go_router_builder and go_router into a single package?

I don't know the details of these packages, but I'm unclear as to why the solution isn't just to have a normal, pub-based dependency on go_router from the other package's example.

johnpryan · 2022-06-01T19:56:10Z

I don't know the details of these packages, but I'm unclear as to why the solution isn't just to have a normal, pub-based dependency on go_router from the other package's example.

If we did that, wouldn't we run into a similar problem? When a change to go_router that breaks go_router_builder is merged and published it wouldn't be detected until the next CI run.

stuartmorgan-g · 2022-06-01T19:59:15Z

Unless the dependency were an any dependency—which it should not be—then the only way that could happen is if someone makes a breaking change to go_router without changing its major version number. Which, yes, would break CI post-publish, but it would also break all clients of go_router in the wild.

johnpryan · 2022-06-01T20:18:16Z

In this case, there wasn't enough test coverage to catch the bug fixed in #2177, but we could certainly make go_router_builder depend on go_router: ^4.0.0 from Pub.

ahmednfwela · 2022-06-02T11:34:42Z

packages/go_router/pubspec.yaml


 environment:
  sdk: ">=2.12.0 <3.0.0"
-  flutter: ">=2.0.0"


shouldn't the package depend on flutter 3.0.0 ? i am getting WidgetsBinding.instance warnings
because of static WidgetsBinding get _binding => WidgetsBinding.instance!;

…ieces (#1653)" (#2183)" This reverts commit d39ffb1.

* Revert "Revert "[go_router] Refactor RouterDelegate into functional pieces (#1653)" (#2183)" This reverts commit d39ffb1. * [go_router] Continue searching for top-level routes if the recursive search doesn't find any * Remove Route.push from demo app * update CHANGELOG.md * Avoid pushing the same page in example bump go_router_builder to 1.0.5 * Change published version to 4.0.0 * Specify dart SDK version >=2.17.0 to avoid analyzer errors Analyzing go_router... error - lib/src/go_route_information_provider.dart:24:41 - A value of type 'WidgetsBinding?' can't be returned from the function '_binding' because it has a return type of 'WidgetsBinding'. - return_of_invalid_type error - lib/src/go_router.dart:51:33 - The property 'platformDispatcher' can't be unconditionally accessed because the receiver can be 'null'. Try making the access conditional (using '?.') or adding a null check to the target ('!'). - unchecked_use_of_nullable_value 2 issues found. * Specify flutter >=3.0.0 in go_router * Apply suggestions from code review Co-authored-by: Loïc Sharma <[email protected]> * Apply suggestions from code review * Update packages/go_router/lib/src/go_route_information_parser.dart Co-authored-by: Loïc Sharma <[email protected]> * Update packages/go_router/lib/src/go_route_information_parser.dart Co-authored-by: Loïc Sharma <[email protected]> * Update packages/go_router/lib/src/go_route_information_parser.dart Co-authored-by: Loïc Sharma <[email protected]> * Update packages/go_router/lib/src/go_route_information_parser.dart Co-authored-by: Loïc Sharma <[email protected]> * Add '/' route to test * add go_route_information_provider_test.dart * Add test for redirect detection in GoRouteInformationParser * format Co-authored-by: Loïc Sharma <[email protected]>

chunhtai mentioned this pull request May 2, 2022

[go_router] Add BuildContext to redirect flutter/flutter#99121

Closed

3 tasks

chunhtai force-pushed the issues/99121-refactor branch 2 times, most recently from 7331b08 to ea2eb4f Compare May 6, 2022 20:55

chunhtai requested a review from johnpryan May 13, 2022 20:32

chunhtai force-pushed the issues/99121-refactor branch from 3081965 to be0f48a Compare May 16, 2022 22:29

johnpryan approved these changes May 23, 2022

View reviewed changes

johnpryan reviewed May 23, 2022

View reviewed changes

packages/go_router/lib/src/go_router.dart Outdated Show resolved Hide resolved

chunhtai force-pushed the issues/99121-refactor branch from be0f48a to 8470008 Compare May 25, 2022 18:08

github-actions bot added the p: go_router label May 25, 2022

chunhtai mentioned this pull request May 26, 2022

[go_router] remove debugLogDiagnostics from GoRouterDelegate flutter/flutter#104470

Closed

[go_router] Refactor RouterDelegate into functional pieces

2a293bb

chunhtai force-pushed the issues/99121-refactor branch from 2a907cf to 2a293bb Compare May 27, 2022 23:32

johnpryan approved these changes May 31, 2022

View reviewed changes

johnpryan merged commit 2c02052 into flutter:main May 31, 2022

stuartmorgan-g pushed a commit that referenced this pull request Jun 1, 2022

Revert "[go_router] Refactor RouterDelegate into functional pieces (#…

85cf1ba

…1653)" This reverts commit 2c02052.

stuartmorgan-g mentioned this pull request Jun 1, 2022

Revert "[go_router] Refactor RouterDelegate into functional pieces" #2183

Merged

stuartmorgan-g added a commit that referenced this pull request Jun 1, 2022

Revert "[go_router] Refactor RouterDelegate into functional pieces (#…

d39ffb1

…1653)" (#2183) This reverts commit 2c02052.

ahmednfwela reviewed Jun 2, 2022

View reviewed changes

johnpryan added a commit that referenced this pull request Jun 2, 2022

Revert "Revert "[go_router] Refactor RouterDelegate into functional p…

45921da

…ieces (#1653)" (#2183)" This reverts commit d39ffb1.

This was referenced Jun 2, 2022

[go_router] Refactor RouterDelegate into functional pieces - second attempt #2188

Closed

Refactor RouterDelegate attempt 2 #2189

Merged

johnpryan mentioned this pull request Jun 2, 2022

Remove path-based dependencies in go_router_builder/example flutter/flutter#105246

Closed

johnpryan added a commit that referenced this pull request Jun 7, 2022

Revert "Revert "[go_router] Refactor RouterDelegate into functional p…

77d799f

…ieces (#1653)" (#2183)" This reverts commit d39ffb1.

johnpryan added a commit that referenced this pull request Jun 14, 2022

Revert "Revert "[go_router] Refactor RouterDelegate into functional p…

82c52fe

…ieces (#1653)" (#2183)" This reverts commit d39ffb1.

	// ignore: unnecessary_non_null_assertion
	static WidgetsBinding get _binding => WidgetsBinding.instance!;
	static WidgetsBinding get _binding => WidgetsBinding.instance;

	final bool replace = type == RouteInformationReportingType.neglect \|\|
	// Avoid adding a new history entry if the route is the same as before.
	final bool replace = type == RouteInformationReportingType.neglect \|\|

[go_router] Refactor RouterDelegate into functional pieces #1653

[go_router] Refactor RouterDelegate into functional pieces #1653

Uh oh!

Conversation

chunhtai commented Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

johnpryan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnpryan May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

johnpryan commented May 26, 2022

Uh oh!

stuartmorgan-g commented Jun 1, 2022

Uh oh!

johnpryan commented Jun 1, 2022

Uh oh!

johnpryan commented Jun 1, 2022

Uh oh!

stuartmorgan-g commented Jun 1, 2022

Uh oh!

johnpryan commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g commented Jun 1, 2022

Uh oh!

johnpryan commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chunhtai commented Apr 27, 2022 •

edited

Loading

johnpryan May 23, 2022 •

edited

Loading

johnpryan commented Jun 1, 2022 •

edited

Loading

johnpryan commented Jun 1, 2022 •

edited

Loading