Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@chunhtai
Copy link
Contributor

@chunhtai chunhtai commented Jan 6, 2021

Description

Previously we throw assertion if the browser receives nav2 updates after it has received nav1 updates, and browser will remain nav1 through out its life time. This means the browser favor nav1 over nav2. This PR updates it so that it will only throw if it receives nav1 update after it has received nav2 update.

In case where users nested a MaterialApp.router inside a MaterialApp, the app will throw assertion error for nav1 update in debug mode, and drop nav1 update silently in release mode.

Related Issues

flutter/flutter#72093

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 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.

Reviewer Checklist

Breaking Change

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

@chinmaygarde chinmaygarde added the platform-web Code specifically for the web engine label Jan 7, 2021
@chunhtai
Copy link
Contributor Author

A friendly bump

Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

The code changes look good to me. But I'm not sure if we should keep the assert or make it a warning. @johnpryan thoughts?

@johnpryan
Copy link

johnpryan commented Jan 20, 2021

If this were to display a warning instead, would the exception from flutter/flutter#72093 get thrown eventually? If that's the case then an assert makes more sense to me, since it's failing earlier.

On the other hand if the exception from flutter/flutter#72093 isn't guaranteed to happen, then a warning might make more sense.

@chunhtai
Copy link
Contributor Author

If this were to display a warning instead, would the exception from flutter/flutter#72093 get thrown eventually? If that's the case then an assert makes more sense to me, since it's failing earlier.

On the other hand if the exception from flutter/flutter#72093 isn't guaranteed to happen, then a warning might make more sense.

If we want to make it a warning, we would change the line that throws this error to print warning instead.

The current situation is that app can ignore this conflict where app uses both Nav1 and Nav2 in the same time. It can complete ignore nav1 update and pretend there is only nav2 update. We don't forbid user from using both nav1 and nav2 together, but we discourage that.

The difference between throwing assertion vs printing warning is that how strong we discourage that.

If we print warning, the app runs fine on both debug and release mode, it will only print warning out in the console. and people may miss if they do not read it carefully.

If we throw, the app runs fine in release mode, but it will crash in debug mode.

@johnpryan
Copy link

Thanks, an assertion sounds right to me here.

@chunhtai 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 Jan 20, 2021
@fluttergithubbot fluttergithubbot merged commit c97bdae into flutter:master Jan 20, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 20, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 20, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 20, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 20, 2021
hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes platform-web Code specifically for the web engine waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants