[go_router]: fix leaking GoRouter.replace and GoRouter.pushReplacement completer, preserve it

[pawlowskim]

When route from the stack is replaced, its completer is lost, and causes futures in routes chain to never finish. This change stores completers from the routes that are replaced and calls their complete method when route that replaces them is popped.

It fixes: flutter/flutter#141251

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

[pawlowskim]

@hangyujin @johnpryan Could you please take a look? Added reviewer is not longer active.

[chunhtai]

This does not look right. the once page-1 is replaced, its future should never be completed. The pop result on line 373 is only meant for page-2. It should not be used to complete page-1.

[pawlowskim]

This does not look right. the once page-1 is replaced, its future should never be completed. The pop result on line 373 is only meant for page-2. It should not be used to complete page-1.

On the other hand, is it good approach to throw any Future into the void (in any case, not only in this particular case)?

This implies that users of go router must carefully design their flow, because feature never ends. And it differs from how the navigator works itself - it completes the future on replacement, which is actually pretty logically.

[chunhtai]

On the other hand, is it good approach to throw any Future into the void (in any case, not only in this particular case)?

No, but sometimes it is unavoidable.

it differs from how the navigator works itself - it completes the future on replacement

Ah you are right, I missed that the navigator completes the previous route for pushReplacement, although the way it completes previous route is different from this pr. We can add a result parameter to pushReplacement to complete previous route. The current implementation uses the same result for both routes seem wrong to me.

The NavigatorState.replace however removes the route without completing it. I think we should keep it that way in go_router

[chunhtai]

The current behavior is working as intended. If a page is removed, its future should never complete

[afpro]

The current behavior is working as intended. If a page is removed, its future should never complete

imagine this

1. use try to use something requires login, so push("/login")
2. after successful login, this use appears as a new user, so pushReplacement("/setup_profile")
3. now, first push("/login") will never complete

this is intended?
then what is the 'right' solution for this?

[chunhtai]

imagine this

1. use try to use something requires login, so push("/login")
2. after successful login, this use appears as a new user, so pushReplacement("/setup_profile")
3. now, first push("/login") will never complete

this is intended? then what is the 'right' solution for this?

assuming we add a result to pushReplacement

void onPress() async {
   var result = await context.push("/login");
   var data = await result;
}

void handleLogin() async {
   var completer = Completer()
   var someData = await context.pushReplacement("/setup_profile", result: completer.future);
   completer.complete(someData);
}

[pawlowskim]

imagine this

1. use try to use something requires login, so push("/login")
2. after successful login, this use appears as a new user, so pushReplacement("/setup_profile")
3. now, first push("/login") will never complete

this is intended? then what is the 'right' solution for this?

assuming we add a result to pushReplacement

void onPress() async {
   var result = await context.push("/login");
   var data = await result;
}

void handleLogin() async {
   var completer = Completer()
   var someData = await context.pushReplacement("/setup_profile", result: completer.future);
   completer.complete(someData);
}

If you add more replace routes down the road, you will need to chain such completers down the flow, otherwise original future from var result = await context.push("/login"); will be lost again.

What if we will reflect navigator behaviour including keeping the futures chain on multiple push replacements? If so, I would remove storing the futures for replace method, but keep it as I did in the PR for pushReplacement?

[chunhtai]

What if we will reflect navigator behaviour including keeping the futures chain on multiple push replacements? If so, I would remove storing the futures for replace method, but keep it as I did in the PR for pushReplacement?

The current implementation may be slightly confused as calling one pop will resolve multiple future with the same result. In your use case, that may be ok. Others may want to have a way to complete these future with different values.

These API suppose to mirror Navigator's.

[afpro]

What if we will reflect navigator behaviour including keeping the futures chain on multiple push replacements? If so, I would remove storing the futures for replace method, but keep it as I did in the PR for pushReplacement?

The current implementation may be slightly confused as calling one pop will resolve multiple future with the same result. In your use case, that may be ok. Others may want to have a way to complete these future with different values.

These API suppose to mirror Navigator's.

if we can't pass result value to original 'push' operation, at least, let original 'await push' get a whatever result value or exception.

[pawlowskim]

I will try to update the PR with the changes @chunhtai suggested - replacing the route will complete the future instead throwing it into the void. Hopefully this week.

[chunhtai]

Hi @pawlowskim I am going to close this pr, feel free to reopen if you have time to work on it.