Implement Router widget and widgets app api #60299

chunhtai · 2020-06-25T18:08:26Z

Description

Introduce router widget and widgets app api, I will include the stock app rewrite in a separate pr once we agree on the api details

stock app rewrite draft chunhtai#2

Related Issues

#45938

Tests

I added the following tests:

See files

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

chunhtai · 2020-07-08T22:57:41Z

packages/flutter/lib/src/widgets/router.dart

@mdebbar @goderbauer as we discussed before, we want to have an api to force create a browser history entry or not.

Having a boolean as an argument after the callback makes it hard to read. I suggest either splitting into two methods or move the boolean argument before the callback.

chunhtai · 2020-07-13T16:53:35Z

a friendly bump

Hixie · 2020-07-14T17:04:44Z

FWIW, it's hard for me to evaluate the API really without the stocks app migration as well.

Hixie · 2020-07-14T17:06:31Z

packages/flutter/lib/src/widgets/app.dart

nit: "if we use"

Hixie · 2020-08-06T17:22:56Z

We should convert it to use the standard codec, if we can. I just don't want to be adding any more places where we're converting data to JSON, it's highly inefficient.

chunhtai · 2020-08-06T20:54:39Z

@Hixie I opened a separate issue about converting navigation channel #63121
I would not do it as part of this pr because it may involve multiple stage migration and this pr is already massive

in the meanwhile, I addressed all other comments. This pr is ready for another look @goderbauer and @Hixie

chunhtai · 2020-08-06T21:39:55Z

packages/flutter/lib/src/widgets/route_notification_messages.dart

@goderbauer
we have to do this check because method channel invoke method will throw if there is no corresponding listener on the engine side.

Should the navigator channel be an OptionalMethodChannel to avoid that problem?

chunhtai · 2020-08-06T21:50:18Z

packages/flutter/lib/src/services/system_navigator.dart

I have to reuse the old method until the engine side method is in place otherwise it will throw an exception.

Instead of duplication the code, forward the call to routeUpdated?

goderbauer

Just some minor comments.

goderbauer · 2020-08-07T18:13:42Z

packages/flutter/lib/src/services/system_channels.dart

... are stored ...

goderbauer · 2020-08-07T18:16:16Z

packages/flutter/lib/src/widgets/route_notification_messages.dart

Should the navigator channel be an OptionalMethodChannel to avoid that problem?

goderbauer · 2020-08-07T18:16:30Z

packages/flutter/lib/src/widgets/route_notification_messages.dart

nit: space between if and (.

goderbauer · 2020-08-07T18:17:04Z

packages/flutter/lib/src/widgets/route_notification_messages.dart

Just remove the else? Same above.

goderbauer · 2020-08-07T18:25:59Z

packages/flutter/lib/src/services/system_navigator.dart

This shouldn't be typed "Object". Maybe take everything that's in the map as named arguments?

goderbauer · 2020-08-07T18:28:39Z

packages/flutter/lib/src/services/system_navigator.dart

Instead of duplication the code, forward the call to routeUpdated?

goderbauer · 2020-08-07T18:29:53Z

packages/flutter/lib/src/services/system_navigator.dart

Make these named arguments?

goderbauer · 2020-08-07T19:41:52Z

Looks like there are some doc errors.

The StandartMessageCodec/JSON question aside, this LGTM.

…dressing doc comments

chunhtai · 2020-08-07T21:22:48Z

packages/flutter/lib/src/widgets/route_notification_messages.dart

@@ -1,41 +0,0 @@
-// Copyright 2014 The Flutter Authors. All rights reserved.


I just realized we are not exposing this api in the package level, we should be good to remove this without a breaking change

chunhtai · 2020-08-07T22:08:40Z

addressed all comments, and also updated the stock app rewrite pr https://github.com/chunhtai/flutter/pull/2/files

goderbauer

LGTM

Hixie · 2020-08-28T22:15:44Z

packages/flutter/lib/src/widgets/app.dart

+    Widget routing;
+    if (_usesRouter) {
+      assert(_effectiveRouteInformationProvider != null);
+      routing = Router<dynamic>(


i missed that we were creating this with dynamic as the type here. That seems to defeat the purpose of having a T type argument on Router...

Not sure what the right answer is, but we should probably find a better solution than defining the API with T then ignoring it.

(I noticed this because we're changing dynamic to Object as part of the NNBD transition.)

I was struggling hard on this one too, you can not set T for a single constructor, and it is weird to define on the class level because non router constructor does not care. we could separate out router constructor to a class or method on its own, but that seems to add a whole lot of complexity to the code. For now, it will let you initialize with different type of route information parser and router delegate, but it will throw on build

fluttergithubbot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels Jun 25, 2020

googlebot added the cla: yes label Jun 25, 2020

chunhtai force-pushed the nav-refactor-router-alone branch from ea1e776 to df72c94 Compare June 30, 2020 00:23

chunhtai changed the title ~~[WIP]Implement Router widget and widgets app api~~ Implement Router widget and widgets app api Jun 30, 2020

chunhtai removed f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. work in progress; do not review labels Jun 30, 2020

fluttergithubbot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. work in progress; do not review labels Jun 30, 2020

chunhtai removed f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jun 30, 2020

chunhtai mentioned this pull request Jul 6, 2020

Fix transition & back button issues DanTup/tab_route_test#1

Closed

chunhtai force-pushed the nav-refactor-router-alone branch from 43dca20 to 25630c6 Compare July 8, 2020 20:12

chunhtai marked this pull request as ready for review July 8, 2020 22:55

fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jul 8, 2020

chunhtai requested review from Hixie and goderbauer July 8, 2020 22:55

chunhtai commented Jul 8, 2020

View reviewed changes

chunhtai removed the work in progress; do not review label Jul 8, 2020

chunhtai force-pushed the nav-refactor-router-alone branch from 852af70 to e571129 Compare July 9, 2020 21:35

chunhtai mentioned this pull request Jul 9, 2020

Nav refactor stock chunhtai/flutter#2

Closed

13 tasks

chunhtai mentioned this pull request Jul 14, 2020

Several routes are called on onGenerateRoute when refreshing Flutter Web app #61351

Closed

Hixie reviewed Jul 14, 2020

View reviewed changes

packages/flutter/lib/src/widgets/app.dart Outdated

Copy link

Contributor

Hixie Jul 14, 2020

Choose a reason for hiding this comment

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

nit: "if we use"

johnpryan mentioned this pull request Aug 6, 2020

NoAnimationTransitionDelegate in documentation shows an animation when a route is popped #63104

Closed

chunhtai force-pushed the nav-refactor-router-alone branch from e8dec5b to 824d8f3 Compare August 6, 2020 19:27

cpboyd mentioned this pull request Aug 6, 2020

Different Appbar with same bottomNavigatioBar without nesting Scaffolds #36461

Open

chunhtai force-pushed the nav-refactor-router-alone branch 2 times, most recently from 41afc9e to 8c54425 Compare August 6, 2020 21:48

chunhtai commented Aug 6, 2020

View reviewed changes

goderbauer reviewed Aug 7, 2020

View reviewed changes

chunhtai added 6 commits August 7, 2020 13:46

Implement Router widget and widgets app api

d1b1c16

address pr comment

8427da1

addressing comment

a17a1e8

address pr comments

e13bf1f

refactor route notification messages and didpushrouteinformation + ad…

bff1bed

…dressing doc comments

addressing review comment

269d37f

chunhtai force-pushed the nav-refactor-router-alone branch from b642cf6 to 9a2d2fd Compare August 7, 2020 20:49

deprecates RouteNotificationMessages

0b113e9

chunhtai force-pushed the nav-refactor-router-alone branch from 9a2d2fd to 0b113e9 Compare August 7, 2020 21:17

chunhtai commented Aug 7, 2020

View reviewed changes

goderbauer approved these changes Aug 7, 2020

View reviewed changes

chunhtai added the waiting for tree to go green label Aug 7, 2020

fluttergithubbot merged commit f9fd71b into flutter:master Aug 8, 2020

provokateurin mentioned this pull request Aug 9, 2020

New system channels go-flutter-desktop/go-flutter#503

Closed

Hixie reviewed Aug 28, 2020

View reviewed changes

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020

Implement Router widget and widgets app api (flutter#60299)

2cd0764

ferraridamiano mentioned this pull request Sep 21, 2020

Can't build due to the new Router widget JideGuru/FlutterEbookApp#37

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 2021

		@@ -1,41 +0,0 @@
		// Copyright 2014 The Flutter Authors. All rights reserved.

Implement Router widget and widgets app api #60299

Implement Router widget and widgets app api #60299

Uh oh!

Conversation

chunhtai commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Jul 13, 2020

Uh oh!

Hixie commented Jul 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Aug 6, 2020

Uh oh!

chunhtai commented Aug 6, 2020

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!

goderbauer 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!

goderbauer commented Aug 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Aug 7, 2020

Uh oh!

goderbauer 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chunhtai commented Jun 25, 2020 •

edited

Loading