Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AssertionError by adding routeInformationProvider to MaterialApp.router(go_router_builder/example) #4300

Closed
wants to merge 0 commits into from

Conversation

bisor0627
Copy link
Contributor

@bisor0627 bisor0627 commented Jun 26, 2023

This PR fixes an AssertionError that occurs when building the MaterialApp widget. The error message is as follows:

════════ Exception caught by widgets library ═══════════════════════════════════
The following assertion was thrown building DefaultSelectionStyle:
'package:go_router/src/parser.dart': Failed assertion: line 63 pos 12: 'routeInformation.state != null': is not true.
스크린샷 2023-06-26 오후 4 48 28

This error was due to the omission of routeInformationProvider in the MaterialApp.router setup. The original code was:

@override
Widget build(BuildContext context) => MaterialApp.router(
    routeInformationParser: _router.routeInformationParser,
    routerDelegate: _router.routerDelegate,
    title: _appTitle,
);

To fix this error, I added routeInformationProvider: _router.routeInformationProvider, to MaterialApp.router:

@override
Widget build(BuildContext context) => MaterialApp.router(
    routeInformationProvider: _router.routeInformationProvider,
    routeInformationParser: _router.routeInformationParser,
    routerDelegate: _router.routerDelegate,
    title: _appTitle,
);

With this fix, the AssertionError no longer occurs and the MaterialApp builds without issues.
스크린샷 2023-06-26 오후 5 19 55

Additionally, I considered changing the MaterialApp.router construction from:

@override
Widget build(BuildContext context) => MaterialApp.router(
    routeInformationProvider: _router.routeInformationProvider,
    routeInformationParser: _router.routeInformationParser,
    routerDelegate: _router.routerDelegate,
    title: _appTitle,
);

to

return MaterialApp.router(
    routerConfig: _router,
);

However, as the lib/main.dart file is already set up with the first MaterialApp.router construction, I decided to maintain its original form.

Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.

List which issues are fixed by this PR. You must list at least one issue.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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

@bisor0627 bisor0627 requested a review from chunhtai as a code owner June 26, 2023 08:20
@flutter-dashboard
Copy link

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 (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

@@ -18,6 +18,7 @@ class App extends StatelessWidget {

@override
Widget build(BuildContext context) => MaterialApp.router(
routeInformationProvider: _router.routeInformationProvider,
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use MaterialApp.router(routerConfig: _router)
directly. It looks like we don't have a test for this example, can you add a simple test like other example just to make sure things can compile?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also update other example to use routerConfig as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can add a test for this example to ensure it compiles correctly. I will also update the other examples to use routerConfig. 😀

@bisor0627
Copy link
Contributor Author

bisor0627 commented Jun 27, 2023

Thank you for your feedback, @chunhtai.

I have updated simple_example.dart and other examples to use routerConfig: _router as you suggested.

Additionally, I've added a test for simple_example.dart. For main.dart, the existing widget_test.dart is already serving as its test, so no additional tests were added.

Please review the changes and let me know if anything else is required.
스크린샷 2023-06-27 오전 10 37 09

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

it looks like all_types.dart is not using routerConfig, can you also update that one? beside format, everything else looks good. Thanks for fixing this!

@@ -98,8 +96,7 @@ class LoginRoute extends GoRouteData {
final String? fromPage;

@override
Widget build(BuildContext context, GoRouterState state) =>
LoginScreen(from: fromPage);
Widget build(BuildContext context, GoRouterState state) => LoginScreen(from: fromPage);
Copy link
Contributor

Choose a reason for hiding this comment

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

we use dart format . on this project directly, this may be causing repro-check failures. Can you run dart format to reformat these files?

@bisor0627
Copy link
Contributor Author

bisor0627 commented Jun 28, 2023

I have updated the all_types.dart file to use routerConfig as per your request. The changes have been committed and pushed. Thank you for pointing out the parts I missed.

Additionally, I have attached a screenshot of the test code running after changing to _routerConfig.
스크린샷 2023-06-28 오전 10 06 54
스크린샷 2023-06-28 오전 10 07 06

@bisor0627 bisor0627 closed this Jul 4, 2023
auto-submit bot pushed a commit that referenced this pull request Jul 13, 2023
This PR is an extension of the previous PR #4300 that I submitted. The previous PR, being based on the main branch, was closed and the contents were copied to a new branch in order to make further contributions.�

In the previous PR, an assertion error was fixed by adding `routeInformationProvider` to `MaterialApp.router`. After a series of code reviews, we updated to use routerConfig and added some test codes.

All the contents have been transferred to the current `bugfix/RouterConfig` branch.

---
*Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.*

*List which issues are fixed by this PR. You must list at least one issue.*

*If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants