Skip to content

[go_router]: fix GoRouter.optionURLReflectsImperativeAPIs flag works with new imperative APIs #6236

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

Merged
merged 26 commits into from
Mar 28, 2024

Conversation

anisovdev
Copy link
Contributor

@anisovdev anisovdev commented Mar 1, 2024

After 13.0.0 release of go_router package GoRouter.optionURLReflectsImperativeAPIs is not working correct.
Isn't correct = url in browser doesn't updates after push, example you can see in new test, or in linked issue

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

Pre-launch Checklist

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

@anisovdev anisovdev requested a review from chunhtai as a code owner March 1, 2024 16:16
Copy link

google-cla bot commented Mar 1, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@anisovdev anisovdev changed the title go_router: fix GoRouter.optionURLReflectsImperativeAPIs flag works with new imperative APIs [go_router]: fix GoRouter.optionURLReflectsImperativeAPIs flag works with new imperative APIs Mar 1, 2024
@anisovdev
Copy link
Contributor Author

@chunhtai
can you review this pull request?

@anisovdev
Copy link
Contributor Author

@Hangyujin
Seems like @chunhtai is not online for the last 2 mouth. Can you review this Pull Request?

@hannah-hyj hannah-hyj self-requested a review March 13, 2024 00:45
@anisovdev
Copy link
Contributor Author

@polina-c mb you can help with review this PR?
This is critical issue to me and i want to see this changes on pub.dev as soon as possible

@polina-c
Copy link

@polina-c mb you can help with review this PR? This is critical issue to me and i want to see this changes on pub.dev as soon as possible

@Hangyujin seems to be better person to review this

@hannah-hyj
Copy link
Member

hannah-hyj commented Mar 15, 2024

Hi, this PR looks good to me, thank you for fixing this issue introduced by 13.0.0.

@johnpryan Hi John, can you take a second look at this pr since chun-heng is on vacation? This pr fixes an issue that is introduced by go_router 13.0.0 because 13.0.0 refactored RouteMatchList to be a tree structure.

@hannah-hyj
Copy link
Member

hannah-hyj commented Mar 15, 2024

@anisovdev Can you bump the version to 13.2.2 and also update pubspec.yaml?

Copy link
Member

@hannah-hyj hannah-hyj left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -2,6 +2,7 @@

- Updates minimum supported SDK version to Flutter 3.16/Dart 3.2.
- Fixes memory leaks.
- GoRouter.optionURLReflectsImperativeAPIs now works correctly with new imperative APIs
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This description sounds like you changed the "optionURLReflectsImperativeAPIs" flag. You can update the description like "Fixes restoreRouteInformation issue when GoRouter.optionURLReflectsImperativeAPIs is true and the last match is ShellRouteMatch"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@anisovdev
Copy link
Contributor Author

@Hangyujin
bumped

@hannah-hyj hannah-hyj requested a review from johnpryan March 19, 2024 05:21
@hannah-hyj hannah-hyj added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 20, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 20, 2024
Copy link
Contributor

auto-submit bot commented Mar 20, 2024

auto label is removed for flutter/packages/6236, due to This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

  • Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@hannah-hyj
Copy link
Member

@johnpryan Hi John, can you take a second look at this pr since chun-heng is on vacation? This pr fixes an issue that is introduced by go_router 13.0.0 because 13.0.0 refactored RouteMatchList to be a tree structure.

@anisovdev
Copy link
Contributor Author

@johnpryan Review please this Pull Request

@anisovdev
Copy link
Contributor Author

anisovdev commented Mar 28, 2024

@Hangyujin now this PR has two review

@hannah-hyj hannah-hyj added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 28, 2024
@auto-submit auto-submit bot merged commit c7d30e2 into flutter:main Mar 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 29, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 29, 2024
flutter/packages@924c7e6...51faaa1

2024-03-29 [email protected] [shared_preferences] Update mockito to the new version available 5.2.0 (flutter/packages#6332)
2024-03-29 [email protected] Manual roll Flutter (stable) from 68bfaea to 300451a (2 revisions) (flutter/packages#6433)
2024-03-28 49699333+dependabot[bot]@users.noreply.github.com [image_picker]: Bump androidx.exifinterface:exifinterface from 1.3.6 to 1.3.7 in /packages/image_picker/image_picker_android/android (flutter/packages#5705)
2024-03-28 [email protected] Manual roll Flutter from dbdcead to 89ea492 (54 revisions) (flutter/packages#6431)
2024-03-28 [email protected] [ci] Adds sleep 60s to release action. (flutter/packages#6405)
2024-03-28 [email protected] [go_router]: fix GoRouter.optionURLReflectsImperativeAPIs flag works with new imperative APIs (flutter/packages#6236)
2024-03-28 [email protected] [interactive_media_ads] Reland "Creates and adds the interactive_media_ads plugin #6060" (flutter/packages#6425)
2024-03-28 [email protected] [google_sign_in_ios] Pins GoogleSignIn to 7.0.0 in podspec. (flutter/packages#6430)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
…with new imperative APIs (flutter#6236)

After 13.0.0 release of go_router package `GoRouter.optionURLReflectsImperativeAPIs` is not working correct.
Isn't correct = url in browser doesn't updates after push, example you can see in new test, or in linked issue

[List which issues are fixed by this PR. You must list at least one issue.](flutter/flutter#142053)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: go_router
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants