stock app rewrite using router #63424

chunhtai · 2020-08-10T20:44:17Z

Description

Rewrite the stock app with router widget

Related Issues

#63429

Tests

I added the following tests:

existing test coverage

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

Hixie · 2020-08-21T22:49:18Z

dev/benchmarks/test_apps/stocks/lib/main.dart

-void main() {
-  runApp(StocksApp());
-}
+void main() => runApp(const StocksApp());


what's the benefit of having this in a separate file?

Hixie · 2020-08-21T22:52:28Z

I think this code would benefit from a further separation of the "app state" from the widget code. Right now, it's sort of entangled (a result of the original architecture, my bad), e.g. with the StocksApp widget having static methods, and the data being in the State of that widget, and so on. Does that make sense?

chunhtai · 2020-09-03T22:09:40Z

Thanks for review @Hixie I almost forgot about this PR, i try my best to separate out the stock configuration can you another look?

ido-ran · 2020-09-05T18:51:43Z

I think there is a problem with showing the current location in the browser address bar due to Navigator 2.0 change.
It happened both in this sample app and a smaller test app I've built.

The RouteInformationParser has the function restoreRouteInformation which seems that it is exactly what the browser should show, but it always show #/ at the end of the application address.

The router does update the application correctly if I change the URL but then it reset it back to #/ immediately after.

On the subject but a different topic - is it possible/plan to have Flutter web app use pushstate API?

chunhtai · 2020-09-08T23:44:05Z

I think there is a problem with showing the current location in the browser address bar due to Navigator 2.0 change.
It happened both in this sample app and a smaller test app I've built.

The RouteInformationParser has the function restoreRouteInformation which seems that it is exactly what the browser should show, but it always show #/ at the end of the application address.

The router does update the application correctly if I change the URL but then it reset it back to #/ immediately after.

On the subject but a different topic - is it possible/plan to have Flutter web app use pushstate API?

The web browser does not work until #64494 is fixed

the #/ is due to our location strategy. There is a pr to change that flutter/engine#17829

chunhtai · 2020-09-09T23:55:17Z

@goderbauer @Hixie This is ready for another look

Hixie · 2020-09-16T23:12:17Z

dev/benchmarks/test_apps/stocks/lib/main.dart

+    if (configuration is StockHomePath)
+      return const RouteInformation(location: _kHomePageLocation);
+    if (configuration is StockSymbolPath)
+      return RouteInformation(location: '$_kStockPageLocation?symbol=${configuration.symbol}');


the symbol needs escaping, otherwise you have (in principle, anyway) an injection vulnerability.

(i'm assuming the constants don't need escaping because they're designed to not have anything outside of the local appropriate alphabet)

goderbauer · 2020-09-16T21:14:57Z

dev/benchmarks/test_apps/stocks/lib/main.dart

  }

-  ThemeData get theme {
-    switch (_configuration.stockMode) {
+  ThemeData geThemeFromConfiguration(StockConfiguration configuration) {


Should this be private?

goderbauer · 2020-09-16T21:17:43Z

dev/benchmarks/test_apps/stocks/lib/main.dart

+  }
+
+  bool _handlePopPage(Route<dynamic> route, dynamic result) {
+    // _handlePopPage should not be called on the home page because the


"should not" or "will not"? (i.e. does the PopNavigatorRouterDelegateMixin make sure that this is never called?)

goderbauer · 2020-09-16T21:19:29Z

dev/benchmarks/test_apps/stocks/lib/main.dart

+
+  @override
+  Widget build(BuildContext context) {
+    assert(routerState.debugInitialized);


Instead of introducing this extra "debugInitialized" field, could this just be

Suggested change

assert(routerState.debugInitialized);

assert(reouterState.routePath != null);

goderbauer · 2020-09-16T21:26:28Z

dev/benchmarks/test_apps/stocks/lib/stock_home.dart

-    if (widget.updater != null)
-      widget.updater(widget.configuration.copyWith(stockMode: value));
+    final StockState state = StockStateScope.of(context);
+    state.updateConfiguration(StockStateScope.configurationOf(context).copyWith(stockMode: value));


This pattern seems pretty common. Maybe StockState should have a method like this:

state.updateConfiguration(stockMode: value);

goderbauer · 2020-09-16T21:29:07Z

dev/benchmarks/test_apps/stocks/lib/stock_state.dart

+  bool get debugInitialized {
+    bool answer;
+    assert((){
+      answer = routePath != null;
+      return true;
+    }());
+    return answer;
+  }


See comment above, I think we can just remove this to simplify things?

goderbauer · 2020-09-16T23:13:43Z

dev/benchmarks/test_apps/stocks/lib/stock_state.dart

+  static StockData stockDataOf(BuildContext context) {
+    return context.findAncestorWidgetOfExactType<StockStateScope>().stocks;
+  }


This only works in this toy example because the stock data never changes, right? If it would, you'd want a dependency to rebuild whenever the data changes, no?

Hixie · 2020-09-16T23:31:57Z

dev/benchmarks/test_apps/stocks/lib/stock_state.dart

+      notifyListeners();
+    }
+  }
+}


would it make sense to replace the routePath getter here with something like:

bool get showSettings => ...; // if true, we show the settings page; otherwise: String get symbol => ...; // if non-null, we show the symbol information page; otherwise, show home page.

It seems like it would be simpler (far fewer classes, for example). You'd have to have a function to set the current mode to replace the set routePath setter, maybe something like;

void updateState({ bool showSettings, String symbol }) { assert(!showSettings || symbol == null); // ...set private fields that affect the getters above... notifyListeners; }

Since this is app is an example for developer to use router, i think the current approach of using the route class is better. if we convolute route classes into only a few settings, thing may become out of control if developer have more route.

For example, navigating between page if we use these property will be

showSettings= true; symbol =null // go to setting page showSettings = false; symbol ='AAPL' // got to symbol page showSettings= false; symbol =null // go to home page

vs

routePath = StockSettingPath() // go to setting page routePath = StockSymbolPath(symbol: 'AAPL') // go to symbol page routePath = StockHomePath() // go to Home page

I think the latter is more descriptive and easier to maintain

it is almost like we are asking user to figure out
A = 1100
B = 0110
C = 0101
D = 1001

now X xor Y = 0011 , figure what is X and Y

Hixie · 2020-09-16T23:33:06Z

dev/benchmarks/test_apps/stocks/lib/stock_state.dart

+enum StockMode { optimistic, pessimistic }
+enum BackupMode { enabled, disabled }
+
+class StockConfiguration {


we could leave this in stock_types.dart to make the patch simpler; maybe I'm missing a good reason to move it here though?

Isn't this part of the appstate? wouldn't it be good to group all the app state together in one file?

I separated out the router state to be in the routing/router_state.dart

+1 to keeping this in the old file for now.

Hixie · 2020-09-16T23:33:21Z

Sorry for delay in reviewing this.

skia-gold · 2020-09-16T23:54:43Z

Gold has detected about 3 untriaged digest(s) on patchset 5.
View them at https://flutter-gold.skia.org/cl/github/63424

chunhtai · 2020-09-29T01:18:57Z

I added the following change

separate out the router related class into routing/
Make use of browser state to store the tab index in home page

ready for another look

goderbauer · 2020-09-29T21:52:57Z

dev/benchmarks/test_apps/stocks/lib/routing/router.dart

+class _RouterConfiguration {
+  _RouterConfiguration(this.path, this.browserState);
+  StockRoutePath path;
+  Map<String, dynamic> browserState;


What's this?

Ah, it's to store the active tab. Shouldn't that be a field on the appropriate StockRoutePath instead of just a generic Map<String, dynamic>?

If this information is part of StockHomePath, it will be problematic when user currently on StockSymbolPath and go back to StockHomePath using Navigator.pop. In the _handlePopPage, we don't know what is the current tab for StockHomePath.

This makes me think, anything that does not reflect to the URL (i.e. browser state) should be global to all the routes.

I agree with @goderbauer that this is suboptimal. I don't think we should be encouraging people to use open maps in this way. The whole point of the router API is to make the data structured.

But how do we solve the issue when user uses navigator.pop to go back from stocksymbolpath to stockhomepath?
If the tab index is only store of StockHomePath, in the _handlePopPage I only have the information about stocksymbolpath and i do not know how to set the tab index in StockHomePath.

We can probably make the StockHomePath to take optional tab index. If it is not provided, it will use whatever in its current state. What do you think?

making the browser state global is a a compromise between imperative API vs browser routing. If we are sure user do not mix one or another, we can make it a state inside StockHomePath. Or we can simply remove this functionality and let developer figure out what is the best way to store browser state. What do you think?

goderbauer · 2020-09-29T21:58:05Z

dev/benchmarks/test_apps/stocks/lib/routing/router.dart

+    final List<Page<void>> pages = <Page<void>>[
+      StockHomePage()
+    ];
+
+    if (routerState.routePath is StockSettingsPath) {
+      pages.add(StockSettingsPage());
+    }
+
+    if (routerState.routePath is StockSymbolPath) {
+      final StockSymbolPath routePath = routerState.routePath as StockSymbolPath;
+      pages.add(StockPage(routePath.symbol));
+    }


This could use collection if, e.g.

pages = [
FooPage(),
if (...) BarPage(),
]

goderbauer · 2020-09-29T22:02:15Z

dev/benchmarks/test_apps/stocks/lib/stock_state.dart

+enum StockMode { optimistic, pessimistic }
+enum BackupMode { enabled, disabled }
+
+class StockConfiguration {


+1 to keeping this in the old file for now.

chunhtai · 2020-10-01T16:38:35Z

ready for another look.

chunhtai · 2020-10-19T16:22:20Z

@goderbauer a gentle ping

Hixie · 2020-10-19T18:07:17Z

dev/benchmarks/test_apps/stocks/lib/main.dart

  }

-  ThemeData get theme {
-    switch (_configuration.stockMode) {
+  ThemeData _geThemeFromConfiguration(StockConfiguration configuration) {


_geThemeFromConfiguration -> _getThemeFromConfiguration

Hixie · 2020-10-19T18:10:40Z

dev/benchmarks/test_apps/stocks/lib/routing/router.dart

+class _RouterConfiguration {
+  _RouterConfiguration(this.path, this.browserState);
+  StockRoutePath path;
+  Map<String, dynamic> browserState;


I agree with @goderbauer that this is suboptimal. I don't think we should be encouraging people to use open maps in this way. The whole point of the router API is to make the data structured.

Hixie · 2020-10-19T20:11:53Z

I tried to port Stocks to Router myself, to see what I would come up with, but before I got started I realised we don't seem to have any documentation on how to get started with Router. It would be great to have something on our Web site in the cookbook or similar that talks about how to build an app with Router, and then link to it from MaterialApp.router, Router, RouterDelegate, etc.

chunhtai · 2020-10-19T20:18:32Z

I tried to port Stocks to Router myself, to see what I would come up with, but before I got started I realised we don't seem to have any documentation on how to get started with Router. It would be great to have something on our Web site in the cookbook or similar that talks about how to build an app with Router, and then link to it from MaterialApp.router, Router, RouterDelegate, etc.

We have a blog post about it https://medium.com/flutter/learning-flutters-new-navigation-and-routing-system-7c9068155ade
We also plan to use this app as an sample app that uses Router
Maybe we should create a codelab or something? What do you think @johnpryan ?

johnpryan · 2020-10-19T22:19:49Z

I think a cookbook article is a good idea. The article has a codelab/exercise in it already.

orestesgaolin · 2020-10-19T23:03:14Z

As developer I would love to see sample and simple Router or RouterDelegate implementation in the code documentation similarly as there is NoAnimationTransitionDelegate in TransitionDelegate. In most cases this should be just enough to start developing on your own.

I tried to understand how to use Router and came up with this example using ChangeNotifier and Provider. The web demo can be found here.

flutter-dashboard bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Aug 10, 2020

johnpryan mentioned this pull request Aug 17, 2020

Router #45938

Closed

3 tasks

chunhtai force-pushed the nav-refactor-stock branch from c5dd346 to 1c91069 Compare August 20, 2020 23:50

googlebot added the cla: yes label Aug 20, 2020

chunhtai requested review from goderbauer and Hixie August 20, 2020 23:51

Hixie reviewed Aug 21, 2020

View reviewed changes

goderbauer mentioned this pull request Sep 14, 2020

Navigator 2.0 Remaining Work #65776

Closed

6 tasks

This was referenced Sep 14, 2020

Makes Router more accessible to developers #63429

Closed

Integrate Navigator 2.0 and state restoration for the web #65777

Open

Browser Forward button not enabled #32248

Closed

chunhtai requested a review from Hixie September 15, 2020 17:13

Hixie reviewed Sep 16, 2020

View reviewed changes

goderbauer reviewed Sep 16, 2020

View reviewed changes

Hixie reviewed Sep 16, 2020

View reviewed changes

chunhtai force-pushed the nav-refactor-stock branch from f763d4a to fb2a57c Compare September 16, 2020 23:38

chunhtai force-pushed the nav-refactor-stock branch 3 times, most recently from 2589b82 to fae77c7 Compare September 28, 2020 20:56

chunhtai requested review from goderbauer and Hixie September 29, 2020 01:17

goderbauer reviewed Sep 29, 2020

View reviewed changes

chunhtai added 6 commits September 30, 2020 15:26

stock app rewrite using router

923005d

separating out stock state

507e387

fix whitespace

b3863dd

fix format

0d968c5

fix tests

1e3fb42

refactor app state and add browser history state for home tab

1a9f8bb

chunhtai force-pushed the nav-refactor-stock branch 2 times, most recently from 09d17d9 to 209b2e1 Compare September 30, 2020 23:04

addressing comment

ccdc65f

chunhtai force-pushed the nav-refactor-stock branch from 209b2e1 to ccdc65f Compare October 1, 2020 00:01

chunhtai requested a review from goderbauer October 1, 2020 16:37

mdebbar mentioned this pull request Oct 16, 2020

Prepare url_launcher for the Link widget flutter/plugins#3154

Merged

Hixie reviewed Oct 19, 2020

View reviewed changes

chunhtai closed this Mar 10, 2021

	assert(routerState.debugInitialized);
	assert(reouterState.routePath != null);

stock app rewrite using router #63424

stock app rewrite using router #63424

Uh oh!

Conversation

chunhtai commented Aug 10, 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!

Hixie commented Aug 21, 2020

Uh oh!

chunhtai commented Sep 3, 2020

Uh oh!

ido-ran commented Sep 5, 2020

Uh oh!

chunhtai commented Sep 8, 2020

Uh oh!

chunhtai commented Sep 9, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Sep 16, 2020

Uh oh!

skia-gold commented Sep 16, 2020

Uh oh!

chunhtai commented Sep 29, 2020 • 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!

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!

chunhtai Oct 19, 2020 • 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

chunhtai commented Aug 10, 2020 •

edited

Loading

chunhtai commented Sep 29, 2020 •

edited

Loading

chunhtai Oct 19, 2020 •

edited

Loading

chunhtai commented Oct 19, 2020 •

edited

Loading

johnpryan commented Oct 19, 2020 •

edited

Loading