feat!: migrate to onDidRemovePage

[JaspervanRiet]

Description

This PR migrates DuckRouter to the new Page API for popping pages, see https://docs.flutter.dev/release/breaking-changes/navigator-and-page-api.

The change has a fundamental impact on us. The suggestion made in the breaking change doc is to remove the page from your list of pages. In our case, that would be in DuckNavigator. The problem we have is that DuckNavigator is that's not where we keep state. That's because we use Navigator both in nested and un-nested contexts. We keep state in the DuckShell and in the DuckRouterDelegate. But at that level, we have abstracted away our use of Page. So when Navigator returns us the page that's been popped, we don't really have a use for it, and we cannot find the corresponding Location for it. At that point we have three choices:

• When the callback gets triggered, just pop stack
• Try to find the location
• Change our mapping of _pages in DuckNavigator

This PR has gone for the second option, mandating the name field on any Page objects given to DuckRouter and using that to find the Location by path. I tried option 3 too, by implementing a mapping of Location and Page objects in _updatePages, but tests started failing as soon as I did that.

The stance I have taken is that, just like our previous implementation, this system should be hidden from the user of DuckRouter. This was mostly possible, with one exception: If a user uses a totally custom Page, i.e. a custom override of Page without using DuckPage, they are not required by the Page API to add a name (it's optional). Thus, in that case we are forced to throw an error.

Related Issues

26

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

• [X] Yes, this is a breaking change.
• [ ] No, this is not a breaking change.

[JaspervanRiet]

Note: CI is failing because our runner is on the wrong Flutter version

[DelcoigneYves]

Thanks for your work!

I do wonder if the approach is correct though. You are still depending on the previous onPopPage property, which is deprecated and might be removed in the future. Shouldn't we try to update our implementation to fully remove the usage of this property?

[JaspervanRiet]

I do wonder if the approach is correct though. You are still depending on the previous onPopPage property, which is deprecated and might be removed in the future.

I'd urge you to read navigator.dart again. This mistake is easy to make because I've essentially abstracted the logic so that we still have an onPopPage, but we don't use it from the Flutter API anymore! This implementation is free of deprecated code.

[DelcoigneYves]

Silly me, indeed I was confused and looking at the DuckNavigator constructor! Very nice. I'm just still checking how a fully custom page is working now, everything else already looks good!

[DelcoigneYves]

Hey @JaspervanRiet I believe I found a shortcoming in the implementation.

In your description, you are mentioning a fully custom page needs to have a name, or it won't be possible to find the Location. This is true, but is not the main issue: the main issue is that, when providing a custom page in the pageBuilder getter, you also need to be able to set the onPopInvoked property on that page with the correct callback.

I tested with your current implementation, and when overriding the pageBuilder in a Location to return a CupertinoPage, and then popping this page with a result, the result is never returned to the caller (as the completer is never called).

This was also the reason why, in my PR #27, I made the onPopInvoked in DuckRouter public, so this could be used by custom pages.

Ideally we could have some way to customize the provided custom page and add this property behind the scenes, but I haven't found a way yet to do so. Can you think of anything?

[JaspervanRiet]

Thank you so much for doing that work. I fear we're going to have to declare a custom Page type that extends Page. That one can then require name, as well as handling this stuff in the background.

[DelcoigneYves]

I was thinking about that too, but then it becomes more difficult to use the MaterialPage or CupertinoPage classes… I like using those, if I want the Cupertino trnasitions on Android as well, or when I want to show a page as a modal (I believe it’s the fullScreenDialog property). Requiring a custom page prohibits using those pages. Sorry for being the voice of negativity here 😅

What we also could do to mitigate above issue, is check if the provided page is a Material or Cupertino Page, and create a new instance of it with the first instance’s parameters. Feels a bit hacky while I’m writing this, and all parameters should be exposed as properties again to get this working 😁

Do you also create pages in this way, or how would you approach cases like these?