Implement browser history class for router widget #20794

chunhtai · 2020-08-26T19:33:07Z

Description

Refactor the existing BrowserHistory class and implements a new class to handle the router widget.

Related Issues

Fixes flutter/flutter#64494

Tests

I added the following tests:

TBD after people agree with this approach

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 C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
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

flutter-dashboard · 2020-08-26T19:33:09Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

chunhtai · 2020-08-26T19:52:56Z

hi @goderbauer @mdebbar, I have this draft to implement engine side changes for router widget. Unfortunately I have to change the API of BrowserHistory a bit so that i can switch between two mechanisms when it receive different notifications. Let me know if this is the direction we want to go, i will write test later if so.

mdebbar

Overall, this is heading in a good direction.

I had one concern about tearing off location strategies. If the app pushes route names /foo and /bar, the hash location strategy will push 2 history entries with route names /#/foo and /#/bar. Now let's say the app switches to a different location strategy. The entire browser history will be lost.

But after I gave it some thought, I realized that that's okay. We don't expect apps to suddenly change their location strategy. The app should configure the location strategy before the app runs, and that's it. So maybe we should assert somewhere that the location strategy doesn't change anymore?

Another question is: do we even want to keep the SingleEntryBrowserHistory? Should we just replace it with MultiEntriesBrowserHistory for all apps (and in that case, we don't have to worry about switching between history implementations)?

lib/web_ui/lib/src/engine/history.dart

lib/web_ui/lib/src/engine/window.dart

chunhtai · 2020-08-27T19:04:44Z

@mdebbar If we remove the single entry one, we may need to figure out what to do with the information when backward button is pressed. I have an idea that may work except forward.

When the backward button is pushed, we can calculate how many states were poped by using the _flutterHistoryCount and just issue that many of pop to the framework (we may still need a way to figure out whether we are currently using router or navigator).

However, I haven't come up with a way to deal forward button. If we forward more than one state, we need to have a way to know all the intermediate urls to push new routes onto navigator which i am not sure if it is possible?

edit:

I think we can keep an internal copy of states list so that we can figure out all the intermediate urls when doing forward. I think that will work, I will try to update this pr with the new design

chunhtai · 2020-09-10T01:01:28Z

Hi @mdebbar, this is ready for another look!

mdebbar

This is looking really good! All my comments are minor/non-blocking.

mdebbar · 2020-09-10T17:30:01Z

lib/web_ui/lib/src/engine/history.dart

+///   a Router for routing.
+class MultiEntriesBrowserHistory extends BrowserHistory {
+  int _lastSeenSerialCount = 0;
+  int get _getCurrentSerialCount {


Suggestion: remove the get prefix from _getCurrentSerialCount.

mdebbar · 2020-09-10T17:48:54Z

lib/web_ui/lib/src/engine/history.dart

+
+  @override
+  void setRouteName(String? routeName, {dynamic? state}) {
+    if (locationStrategy != null && routeName != null) {


Could you add a comment here explaining why we are ignoring null route names?

mdebbar · 2020-09-10T18:05:22Z

lib/web_ui/lib/src/engine/history.dart

+  @override
+  Future<void> tearDown() async {
+    // Restores the html browser history.
+    if (_hasSerialCount(currentState)) {


If everything goes well, this condition should always be true right? If it's false, that means someone other than us is modifying history state. What do you think about doing an assert instead?

mdebbar · 2020-09-10T18:08:57Z

lib/web_ui/lib/src/engine/history.dart

+      }
+      // Unwrap state.
+      locationStrategy!.replaceState(
+        currentState['state'],


Is it possible for currentState to be null here?

it is impossible otherwise the _hasSerialCount(currentState) will be false

currentState here is different from the one we passed to _hasSerialCount above.

ah yes, you are right, but it still shouldn't be null because we also wrap it with serial count 0.

I will add some assert

mdebbar · 2020-09-10T18:21:43Z

lib/web_ui/lib/src/engine/window.dart

+    final LocationStrategy? strategy = _browserHistory.locationStrategy;
+    await _browserHistory.setLocationStrategy(null);
+    _browserHistory = MultiEntriesBrowserHistory();
+    await _browserHistory.setLocationStrategy(strategy);


No need to do this if strategy is null?

fluttergithubbot · 2020-09-10T22:00:02Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Linux Web Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Mac iOS Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Mac Web Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Windows Web Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

…gine#20794)

venkatd · 2020-11-25T12:48:15Z

@chunhtai @mdebbar I'm on the latest master with Navigator 2.0 and I am getting the following when running chrome in debug mode when I navigate to a link. The app continues to work but the url doesn't update.

══╡ EXCEPTION CAUGHT BY FLUTTER FRAMEWORK ╞═════════════════════════════════════════════════════════
The following assertion was thrown:
Assertion failed: org-dartlang-sdk:///flutter_web_sdk/lib/_engine/engine/window.dart:102:16
browserHistory is MultiEntriesBrowserHistory
is not true
When the exception was thrown, this was the stack:
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 216:49  throw_
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 24:3    assertFailed
lib/_engine/engine/window.dart 102:16                                         handleNavigationMessage
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 84:54            runBody
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 123:12           _async
lib/_engine/engine/window.dart 90:39                                          handleNavigationMessage
lib/_engine/engine/platform_dispatcher.dart 411:19                            [_sendPlatformMessage]
lib/_engine/engine/platform_dispatcher.dart 216:5                             sendPlatformMessage
packages/flutter/src/services/binding.dart 259:36                             [_sendPlatformMessage]
packages/flutter/src/services/binding.dart 308:12                             send
packages/flutter/src/services/platform_channel.dart 149:52                    _invokeMethod
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 84:54            runBody
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 123:12           _async
packages/flutter/src/services/platform_channel.dart 147:30                    [_invokeMethod]
packages/flutter/src/services/platform_channel.dart 475:3                     [_invokeMethod]
packages/flutter/src/services/platform_channel.dart 462:18                    invokeMethod
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 84:54            runBody
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 123:12           _async
packages/flutter/src/services/platform_channel.dart 461:29                    invokeMethod
packages/flutter/src/services/system_navigator.dart 45:31                     routeInformationUpdated
packages/flutter/src/widgets/router.dart 1192:21                              routerReportsNewRouteInformation
packages/flutter/src/widgets/router.dart 468:20                               [_reportRouteInformation]
packages/flutter/src/scheduler/binding.dart 1119:15                           [_invokeFrameCallback]
packages/flutter/src/scheduler/binding.dart 1065:9                            handleDrawFrame
packages/flutter/src/scheduler/binding.dart 973:5                             [_handleDrawFrame]
lib/_engine/engine/platform_dispatcher.dart 892:13                            invoke
lib/_engine/engine/platform_dispatcher.dart 145:5                             invokeOnDrawFrame
lib/_engine/engine.dart 244:45                                                <fn>
dart-sdk/lib/async/zone.dart 1202:13                                          _rootRunUnary
dart-sdk/lib/async/zone.dart 1097:19                                          runUnary
dart-sdk/lib/async/zone.dart 1002:7                                           runUnaryGuarded
dart-sdk/lib/async/zone.dart 1039:26                                          <fn>

If I build the app with flutter build web all seems to work.

Doctor summary (to see all details, run flutter doctor -v):
[✓] Flutter (Channel master, 1.24.0-8.0.pre.366, on Mac OS X 10.14.6 18G6032 darwin-x64, locale en-US)
[✓] Android toolchain - develop for Android devices (Android SDK version 28.0.3)
[!] Xcode - develop for iOS and macOS (Xcode 11.3.1)
    ! CocoaPods 1.8.4 out of date (1.9.0 is recommended).
        CocoaPods is used to retrieve the iOS and macOS platform side's plugin code that responds to your plugin usage on the Dart side.
        Without CocoaPods, plugins will not work on iOS or macOS.
        For more info, see https://flutter.dev/platform-plugins
      To upgrade see https://guides.cocoapods.org/using/getting-started.html#installation for instructions.
[✓] Chrome - develop for the web
[✓] Android Studio (version 3.4)
[✓] IntelliJ IDEA Community Edition (version 2020.1.4)
[✓] VS Code (version 1.51.1)
[✓] Connected device (3 available)

! Doctor found issues in 1 category.

Any ideas what might be causing the issue and what to investigate further? Should I create this as a separate issue? I ran into this thread by searching for MultiEntriesBrowserHistory.

chunhtai · 2020-11-25T17:23:19Z

@venkatd this happen when you have a router and a navigator that send message to engine at the same time.
If you use router, none of your navigator should send message. A navigator will send message if it is created through MaterialApp, or it has https://api.flutter.dev/flutter/widgets/Navigator/reportsRouteUpdateToEngine.html set to true

venkatd · 2020-11-25T18:37:11Z

@chunhtai thanks for clarifying. Just so I understand, I should not instantiate a Navigator anywhere. Even simply calling Navigator.of(context) would cause conflicts with the router, correct?

I have removed all references to Navigator and my app is declared as follows:

    return MaterialApp.router(
      title: 'Turtle',
      debugShowCheckedModeBanner: false,
      theme: turtleMaterialTheme,
      routerDelegate: routerDelegate,
      routeInformationParser: TurtleRouteInfoParser(),
    );

However, I continue to get the exception in debug mode.

chunhtai · 2020-11-25T18:45:11Z

Using navigator should be fine, as long as they don't have https://api.flutter.dev/flutter/widgets/Navigator/reportsRouteUpdateToEngine.html set to true.
Can you create a new issue to track this? It seems like a bug.

venkatd · 2020-11-25T18:48:50Z

Sure let me try to create a reproducible example and I will file a bug report. (And FYI I don't have that reportsRouteUpdateToEngine set to true explicitly anywhere and it looks like it defaults to false if unset.)

Edit @chunhtai turns out it was a bug on my end as I was mistakenly instantiating a Navigator instance. Would you like me to create an issue to improve the error message?

googlebot added the cla: yes label Aug 26, 2020

auto-assign bot requested a review from liyuqian August 26, 2020 19:33

chunhtai requested review from mdebbar and goderbauer August 26, 2020 19:48

mdebbar reviewed Aug 27, 2020

View reviewed changes

chunhtai mentioned this pull request Aug 28, 2020

Makes Router more accessible to developers flutter/flutter#63429

Closed

7 tasks

chunhtai added 7 commits September 2, 2020 16:30

Implement browser history class for router widget

ea738b7

update

7ebf300

remove question mark

42e2a29

fix typo

54f1b5f

fix more comments

722d5c6

addressing comment

4c28fd0

update

df5c426

chunhtai force-pushed the issues/64494 branch from d998767 to df5c426 Compare September 3, 2020 18:35

chunhtai added 5 commits September 3, 2020 15:19

fix test

9251d47

temp

6cb2797

refactor a lot of stuff

ef78d7f

fix format

3e9bbc0

fix test

ef4d883

mdebbar approved these changes Sep 10, 2020

View reviewed changes

addressing comments and make page refresh more robust

c7d7900

chunhtai added waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. and removed waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. labels Sep 10, 2020

add assert

7375381

chunhtai added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Sep 10, 2020

zanderso added the platform-web Code specifically for the web engine label Sep 10, 2020

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Sep 10, 2020

fixing test

08043ae

chunhtai added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Sep 10, 2020

fluttergithubbot merged commit 833c6a2 into flutter:master Sep 11, 2020

engine-flutter-autoroll mentioned this pull request Sep 11, 2020

Roll Engine from e831433a20ca to bdaac368f85f (13 revisions) flutter/flutter#65607

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 11, 2020

833c6a2 Implement browser history class for router widget (flutter/en…

bbd023a

…gine#20794)

goderbauer mentioned this pull request Sep 14, 2020

Navigator 2.0 Remaining Work flutter/flutter#65776

Closed

6 tasks

Implement browser history class for router widget #20794

Implement browser history class for router widget #20794

Uh oh!

Conversation

chunhtai commented Aug 26, 2020

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

flutter-dashboard bot commented Aug 26, 2020

Uh oh!

chunhtai commented Aug 26, 2020

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

chunhtai commented Sep 10, 2020

Uh oh!

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

fluttergithubbot commented Sep 10, 2020

Uh oh!

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

Uh oh!

Uh oh!

chunhtai commented Nov 25, 2020

Uh oh!

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

Uh oh!

Uh oh!

chunhtai commented Nov 25, 2020

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

chunhtai commented Aug 27, 2020 •

edited

Loading

venkatd commented Nov 25, 2020 •

edited

Loading

venkatd commented Nov 25, 2020 •

edited

Loading

venkatd commented Nov 25, 2020 •

edited

Loading