Skip to content

Refactor RouterDelegate attempt 2 #2189

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.

Sign up for GitHub

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

Jump to bottom
Merged
johnpryan merged 18 commits into from
Jun 15, 2022
Merged

Refactor RouterDelegate attempt 2 #2189

johnpryan merged 18 commits into from
Jun 15, 2022

Conversation

@johnpryan
Copy link
Contributor

@johnpryan johnpryan commented Jun 2, 2022

Reverts #2183 to re-land #1653 with #2177 included

@johnpryan johnpryan changed the title Refactor routerdelegate 2 Refactor RouterDelegate attempt 2 Jun 2, 2022
@johnpryan johnpryan mentioned this pull request Jun 2, 2022
Closed
@johnpryan johnpryan force-pushed the refactor-routerdelegate-2 branch 2 times, most recently from f5b85e3 to 56056d8 Compare June 7, 2022 17:17
@johnpryan
Copy link
Contributor Author

johnpryan commented Jun 7, 2022

@stuartmorgan this change requires Flutter 3.0, which means that these CI steps are failing:

Does this CI step need to pass before we land this change?

@stuartmorgan-g
Copy link
Collaborator

stuartmorgan-g commented Jun 8, 2022 •
edited
Loading

Does this CI step need to pass before we land this change?

Every CI step needs to land before landing a change.

Per https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#when-to-update-the-required-version (second bullet point) all you need to do is express the correct Flutter constraint to avoid those steps failing. (Currently the tooling only looks at Flutter constraints, not Dart constraints; it's something we should fix at some point, but in this case the package already requires Flutter so a Flutter constraint shouldn't have any downsides.)

loic-sharma
loic-sharma reviewed Jun 9, 2022
View reviewed changes
loic-sharma
loic-sharma reviewed Jun 9, 2022
View reviewed changes
loic-sharma
loic-sharma reviewed Jun 9, 2022
View reviewed changes
loic-sharma
loic-sharma reviewed Jun 10, 2022
View reviewed changes
loic-sharma
loic-sharma reviewed Jun 10, 2022
View reviewed changes
loic-sharma
loic-sharma reviewed Jun 10, 2022
View reviewed changes
loic-sharma
loic-sharma reviewed Jun 10, 2022
View reviewed changes
loic-sharma
loic-sharma reviewed Jun 10, 2022
View reviewed changes
loic-sharma
loic-sharma reviewed Jun 10, 2022
View reviewed changes
loic-sharma
loic-sharma reviewed Jun 10, 2022
View reviewed changes
loic-sharma
loic-sharma reviewed Jun 10, 2022
View reviewed changes
loic-sharma
loic-sharma reviewed Jun 10, 2022
View reviewed changes
loic-sharma
loic-sharma reviewed Jun 10, 2022
View reviewed changes
johnpryan and others added 18 commits June 14, 2022 13:51
@johnpryan
Revert "Revert "[go_router] Refactor RouterDelegate into functional p…
82c52fe 
…ieces (#1653)" (#2183)"

This reverts commit d39ffb1.
@johnpryan
[go_router] Continue searching for top-level routes if the recursive …
5030291 
…search doesn't find any
@johnpryan
Remove Route.push from demo app
e8345c7
@johnpryan
update CHANGELOG.md
ec99867
@johnpryan
Avoid pushing the same page in example
1e13468 
bump go_router_builder to 1.0.5
@johnpryan
Change published version to 4.0.0
367d5ef
@johnpryan
Specify dart SDK version >=2.17.0 to avoid analyzer errors
f4b5bc5 
Analyzing go_router...

  error - lib/src/go_route_information_provider.dart:24:41 - A value of type 'WidgetsBinding?' can't be returned from the function '_binding' because it has a return type of 'WidgetsBinding'. - return_of_invalid_type
  error - lib/src/go_router.dart:51:33 - The property 'platformDispatcher' can't be unconditionally accessed because the receiver can be 'null'. Try making the access conditional (using '?.') or adding a null check to the target ('!'). - unchecked_use_of_nullable_value

2 issues found.
@johnpryan
Specify flutter >=3.0.0 in go_router
f8a1d9c
@johnpryan @loic-sharma
Apply suggestions from code review
0778ecd 
Co-authored-by: Loïc Sharma <[email protected]>
@johnpryan
Apply suggestions from code review
60ead49
@johnpryan @loic-sharma
Update packages/go_router/lib/src/go_route_information_parser.dart
41a71ad 
Co-authored-by: Loïc Sharma <[email protected]>
@johnpryan @loic-sharma
Update packages/go_router/lib/src/go_route_information_parser.dart
da0dd10 
Co-authored-by: Loïc Sharma <[email protected]>
@johnpryan @loic-sharma
Update packages/go_router/lib/src/go_route_information_parser.dart
56894a7 
Co-authored-by: Loïc Sharma <[email protected]>
@johnpryan @loic-sharma
Update packages/go_router/lib/src/go_route_information_parser.dart
798056a 
Co-authored-by: Loïc Sharma <[email protected]>
@johnpryan
Add '/' route to test
f1fc4de
@johnpryan
add go_route_information_provider_test.dart
2385ba6
@johnpryan
Add test for redirect detection in GoRouteInformationParser
7bd410f
@johnpryan
format
fb7c15e
@johnpryan johnpryan force-pushed the refactor-routerdelegate-2 branch from 3d3d53a to fb7c15e Compare June 14, 2022 20:52
loic-sharma
loic-sharma approved these changes Jun 14, 2022
View reviewed changes
Copy link
Member

@loic-sharma loic-sharma left a comment

Choose a reason for hiding this comment

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

Looks good, great work!

@johnpryan johnpryan merged commit f430281 into main Jun 15, 2022
@johnpryan johnpryan deleted the refactor-routerdelegate-2 branch June 15, 2022 16:17
@johnpryan johnpryan mentioned this pull request Jun 21, 2022
Closed
johnpryan added a commit that referenced this pull request Jun 24, 2022
@johnpryan
[go_router] Turn off logging (#2222)
a02be18 
* Call log.clearListeners to disable logging

* Remove debugLogDiagnostics from GoRouterDelegate

fixes #104470

* Add logging test

* format

* Fix copyright header

* Specify types

* Use version 4.0.1

Depends on #2189

* disable logging in tests

* remove print

* Update CHANGELOG.md
@johnpryan johnpryan mentioned this pull request Jul 22, 2022
Closed
rzetzsche
rzetzsche reviewed Sep 6, 2022
View reviewed changes
@rzetzsche rzetzsche mentioned this pull request Sep 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@loic-sharma loic-sharma loic-sharma approved these changes

@kevmoo kevmoo Awaiting requested review from kevmoo

@stuartmorgan-g stuartmorgan-g Awaiting requested review from stuartmorgan-g

+1 more reviewer

@rzetzsche rzetzsche rzetzsche left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

p: go_router_builder p: go_router

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@johnpryan @stuartmorgan-g @loic-sharma @rzetzsche