Navigator: Refactor the imperative api to continue working in the new navigation system

[chunhtai]

Description

This is based on https://github.com/flutter/flutter/pull/30211/files

is the first part of
#45938

[breaking change]
https://groups.google.com/g/flutter-dev/c/A9JSQojOt6I/m/DxCS9eV0GwAJ

Tests

I added the following tests:

Existing test suit should cover the most

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I signed the CLA.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I updated/added relevant documentation (doc comments with ///).
• All existing and new tests are passing.
• The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

• Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
• No, this is not a breaking change.

[fluttergithubbot]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[goderbauer]

(Will continue review after lunch)

[goderbauer]

note: When we support pages, we probably also need a seperate "complete" state, see https://docs.google.com/document/d/1Q0jx0l4-xymph9O6zLaOY4d_f7YFpNWX_eGbzYxr9wY/edit#heading=h.5im0pq8myoqk

[goderbauer]

When you address the changes to this PR can you please push a separate commit for those changes to make it easier to review them? Thanks!

[chunhtai]

@goderbauer This is ready to review, please the the most recent commit. I also include a breaking change email draft. https://docs.google.com/document/d/16T1FCpfP7y2nPAg4PHcQBaXTNZaF3oQjPZJkcnsaKMI/edit#

[goderbauer]

LGTM

[goderbauer]

nit: end with a .

[goderbauer]

nit: remove this blank line.

[slightfoot]

@chunhtai I have just been tracking down a bug caused by this PR.

I understood from the breaking change notice. You have deprecated the isInitialRoute flag but you also removed it's functionality from PageRoute therefore not deprecating it.. but breaking it.

@override
AnimationController createAnimationController() {
	final AnimationController controller = super.createAnimationController();
	if (settings.isInitialRoute)
	  controller.value = 1.0;
	return controller;
}

It is quite vital to be able to stop that initial animation when you for instance use the following code snippet to replace the entire stack with a new root route.

Navigator.of(context).pushAndRemoveUntil(route, (_) => false);

I bring this to your attention to perhaps discuss resolution. In terms of either add the function back in until the flag itself is removed. Or, create a new breaking-change notice announcing this feature removal and adding the function in your own routes as a resolution until the Navigator 2.0 work is complete?

[chunhtai]

@slightfoot
The pushAndRemoveUntil that add the route without an animation is an unexpected behavior, because it should be a "push".
The code you posted in createAnimationController is more like a workaround to get rid of animation when it is an initial route, and that creates an unexpected behavior as you mentioned. In my opinion, this is more of a bug fixes instead of a breaking change.

Back to your question, to achieve the same result without transition, you can use replace to replace the top most route, and use remoteRoute or removeRouteBelow to remove the route below. The problem with this approach is you have to have the reference of the route in current history, you might need to keep track of them yourself which is a bit cumbersome. The use case you demonstrate here is actually the reason why we want to do the refactoring and page api.

Let me know if you have any questions or concerns.

[slightfoot]

@chunhtai as you can see the current implementation of the Navigator does not allow for an alternative to a clear and replace stack. Even if this is a bug-fix, how do we achieve this same behaviour with the new API. You can't remove the isInitialRoute breaking many apps without a replacement feature or migration guide. and the current Migration guide does not cover this scenario.

[orestesgaolin]

The pushAndRemoveUntil that add the route without an animation is an unexpected behavior, because it should be a "push".

@chunhtai I also use this way of resetting the stack. Actually it's very popular on Stack Overflow whenever anyone asks how to do it e.g. here.

In my opinion even though you consider this a bug, many people rely on this behavior. I've heard this solution being recommended on meetups as the "proper one".

[chunhtai]

@orestesgaolin it is ok to rely on this to reset the stack, but it should come with pushing animation.

[orestesgaolin]

@chunhtai yeah, it seems that the names implies the animation. However, I think it should be widely announced that such change in behavior can occur.

[chunhtai]

@orestesgaolin I can add a follow up announcement to the original breaking change, but there is current no migration guide if you want to pushAndRemoveUntil without animation.

[jsnoriegam]

@chunhtai I think if the problem was only the name, what about creating another method with the same functionality but without animations

[chunhtai]

@jsnoriegam That is what i would do if we don't plan to rewrite the whole routing api. However, it does not make sense to extend the imperative api if we plan to encourage people to move to a new system.