[go_router] Add GoRouterState parameters to GoRouterData and rename replace methods #2848

ValentinVignal · 2022-11-25T03:12:39Z

After this is merged, I'll make another PR on [go_router_builder] to replace all the references of the deprecated redirect to redirectWithState

Pre-launch Checklist

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

chunhtai

LGTM

chunhtai · 2022-11-28T21:04:48Z

packages/go_router/CHANGELOG.md

+## 5.3.0
+
+- Adds `redirectWithState` to `GoRouteData`.
+- `GoRouteData.redirect` is now deprecated in favor of `GoRouteData.redirectWithState`.


Suggested change

- `GoRouteData.redirect` is now deprecated in favor of `GoRouteData.redirectWithState`.

- Deprecates `GoRouteData.redirect` in favor of `GoRouteData.redirectWithState`.

I've updated it in ✏️ Better changelog

johnpryan · 2022-11-29T19:13:58Z

Is it possible to keep the name redirect? Why are we shying away from a breaking change here?

chunhtai · 2022-11-29T19:31:44Z

I agree with @johnpryan that we should probably just break it, but I am not sure if we can merge this without breaking the go_router_builder's build. It we can't we would need some sort of soft transition.

chunhtai · 2022-11-29T19:33:43Z

yes it looks like we will probably be fine, all the reference in go_router_builder is doc. Let's just do a breaking change and write a migration guide and bump major version

ValentinVignal · 2022-11-30T01:14:49Z

@chunhtai @johnpryan Sure I can make it a breaking change.
In that case,

Should I remove buildPage (deprecated) and rename buildPageWithState into buildPage?
Should I also add the parameter state to build?

chunhtai · 2022-11-30T16:49:43Z

Should I remove buildPage (deprecated) and rename buildPageWithState into buildPage?

We should definitrly remove buildPage, but I am not sure if we should rename it back, it feels it defeat the purpose of the @deprecated. WDYT @johnpryan

Should I also add the parameter state to build?

yes I think we may as well do all of these at once.

…with-state

…eter to all GoRouteData callbacks

ValentinVignal · 2022-12-01T02:44:56Z

yes I think we may as well do all of these at once.

I've introduced the breaking changes in
💥 Remove xxxWithState and adds a the context and the state as a param…

We should definitrly remove buildPage, but I am not sure if we should rename it back, it feels it defeat the purpose of the @deprecated. WDYT @johnpryan

Yes, that is quite true haha. But since we are introducing breaking changes, why not? It also feels a bit weird that we are okay with breaking changes when there is no deprecation warning. But we are not when there was one (even if it was not really correct in this context).

yes it looks like we will probably be fine, all the reference in go_router_builder is doc. Let's just do a breaking change and write a migration guide and bump major version

For this, do you mean I should write a migration guide like this one? https://flutter.dev/go/go-router-v5-1-2-breaking-changes
I might need some indication on how to create/write one

chunhtai · 2022-12-01T17:47:40Z

@ValentinVignal you can make a copy of flutter.dev/go/template and follow the instruction above. once the doc is available through flutter.dev/go, you can then link the flutter.dev/go link to the go_router readme as part of this pr

chunhtai · 2022-12-01T17:50:53Z

you can also use something like kodify to format code block

chunhtai · 2022-12-01T19:31:55Z

also since you are doing it now, might as well consider including renaming in #2846

…with-state # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

ValentinVignal · 2022-12-05T12:14:58Z

@chunhtai I've create a migration guide (see flutter/website#7910)

I've included the breaking change replace -> pushReplacement

…with-state # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

ValentinVignal · 2022-12-14T10:10:08Z

@chunhtai @johnpryan Is there anything else you want me to do on this PR ?

chunhtai

LGTM, can you add the migration guide to the readme?

ValentinVignal · 2022-12-17T16:42:44Z

LGTM, can you add the migration guide to the readme?

Sorry I forgot to do that.
I've included the link in the README in 📝 Add migration guide's link to the read me

johnpryan

LGTM

ValentinVignal added 3 commits November 25, 2022 11:10

✨ Add redirectWithState method to GoRouteData

1a1ba59

✅ Update the tests

da80b4e

⬆️ Increase version number

520c97e

ValentinVignal requested a review from chunhtai as a code owner November 25, 2022 03:12

github-actions bot added the p: go_router label Nov 25, 2022

chunhtai approved these changes Nov 28, 2022

View reviewed changes

chunhtai requested a review from johnpryan November 28, 2022 21:05

✏️ Better changelog

396264f

ValentinVignal added 3 commits December 1, 2022 10:28

Merge remote-tracking branch 'upstream/main' into go-router/redirect-…

03a6dc1

…with-state

💥 Remove xxxWithState and adds a the context and the state as a param…

4e9c6e3

…eter to all GoRouteData callbacks

📝 Update the change logs and version number

42562c9

Merge remote-tracking branch 'upstream/main' into go-router/redirect-…

f1c3e37

…with-state # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

ValentinVignal mentioned this pull request Dec 5, 2022

Add the go router v6 migration guide flutter/website#7910

Merged

3 tasks

ValentinVignal added 2 commits December 5, 2022 20:18

💥 Rename replace into pushReplacement

4fce4f1

📝 Add pushReplacementNamed into change log

a7db130

chunhtai self-requested a review December 8, 2022 22:57

ValentinVignal added 2 commits December 9, 2022 16:26

Merge remote-tracking branch 'upstream/main' into go-router/redirect-…

c3c843f

…with-state # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

Merge remote-tracking branch 'upstream/main' into go-router/redirect-…

cb35611

…with-state # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

ValentinVignal mentioned this pull request Dec 16, 2022

[go_router] When replace is called, reuse the same key when possible #2846

Merged

11 tasks

chunhtai approved these changes Dec 16, 2022

View reviewed changes

johnpryan approved these changes Dec 16, 2022

View reviewed changes

📝 Add migration guide's link to the read me

6e6afbf

ValentinVignal force-pushed the go-router/redirect-with-state branch from 09a2ecb to 6e6afbf Compare December 17, 2022 16:42

johnpryan changed the title ~~[go_router] Add redirectWithState~~ [go_router] Add GoRouterState parameters to GoRouterData methods Dec 19, 2022

johnpryan changed the title ~~[go_router] Add GoRouterState parameters to GoRouterData methods~~ [go_router] Add GoRouterState parameters to GoRouterData and rename replace methods Dec 19, 2022

johnpryan approved these changes Dec 19, 2022

View reviewed changes

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 19, 2022

auto-submit bot merged commit 1cabcfb into flutter:main Dec 19, 2022

ValentinVignal mentioned this pull request Dec 20, 2022

[go_router_builder] Update to go_router 6 #2977

Merged

11 tasks

	- `GoRouteData.redirect` is now deprecated in favor of `GoRouteData.redirectWithState`.
	- Deprecates `GoRouteData.redirect` in favor of `GoRouteData.redirectWithState`.

[go_router] Add GoRouterState parameters to GoRouterData and rename replace methods #2848

[go_router] Add GoRouterState parameters to GoRouterData and rename replace methods #2848

Uh oh!

Conversation

ValentinVignal commented Nov 25, 2022

Pre-launch Checklist

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai Nov 28, 2022

Choose a reason for hiding this comment

Uh oh!

ValentinVignal Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

johnpryan commented Nov 29, 2022

Uh oh!

chunhtai commented Nov 29, 2022

Uh oh!

chunhtai commented Nov 29, 2022

Uh oh!

ValentinVignal commented Nov 30, 2022

Uh oh!

chunhtai commented Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ValentinVignal commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chunhtai commented Dec 1, 2022

Uh oh!

chunhtai commented Dec 1, 2022

Uh oh!

chunhtai commented Dec 1, 2022

Uh oh!

ValentinVignal commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ValentinVignal commented Dec 14, 2022

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

ValentinVignal commented Dec 17, 2022

Uh oh!

johnpryan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chunhtai commented Nov 30, 2022 •

edited

Loading

ValentinVignal commented Dec 1, 2022 •

edited

Loading

ValentinVignal commented Dec 5, 2022 •

edited

Loading