fix(vue-router): wrong location histories when replace

[yoyo930021]

Pull request checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)
• Build (npm run build) was run locally and any changes were pushed
• Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no api changes)
• Build related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

The tab page will not render with enteringViewItem === leavingViewItem

Issue Number: #24226

What is the new behavior?

The location histories will correct, so enteringViewItem !== leavingViewItem.
The tab page will render.

Does this introduce a breaking change?

• Yes
• No

Other information

Fixed #24226

Do I need to add test ?

[yoyo930021]

The blank page is a different bug.

[liamdebeasi]

Thanks for the PR! We appreciate your contribution to Ionic. Before this gets merged, could you please write a test for this?

[tigohenryschultz]

In my opinion it looks like your change would just ignore another lower level problem.

const last = () => locationHistory[locationHistory.length - 1];

The last would still reference the wrong viewItem? Shoulnd't the last item return the last item correctly, and if not was the one that is missing not de-mounted properly?

I'm currently tracking a similar bug where the prevRouteLastPathname points to the wrong component because something was replaced and not demounted.

Also doesn't help that none of the types.ts properties do not have comments so it is hard to tell what is used for what / why. Like why would you need to know the previous routes last path name, which in some cases could be 2 routes ago if something was replaced.

You would think that calling replace should demount the last item.

You can see a similar issue here, the circle in red is the page I navigated to by 'replace' but the current page is not demounted because prevRouteLastPathname is a mess

The IonRouterOutlet.ts/router.ts/locationHistory.ts is a mess. Should be heavily refactored/commented.

In some cases multiple places can trigger a mount on a component, some things are async and await and others are not awaited properly.

Maybe I am wrong but it is hard to tell anything with a codebase that is not commented.

[liamdebeasi]

I took a quick look and noticed that the routeInfo object when clicking the back button in Step 7 in #23873 (comment) shows that the current path name is "/page2" and the previous route name is "/page2". So in other words, it is trying to go back to the same page again. I need to do a deeper dive on why that is getting set.

Regarding location history, the location history is our way of keeping track of navigation items separate from the browser. There are a few reasons for this:

1. The browser does not expose the browser history for (good) security reasons. For context, we only store information regarding route information within the Ionic app. No personal information is stored and this information is not persisted when you close/reload the page.
2. The browser navigation is linear whereas tabs navigation in mobile apps is not linear. So sometimes we need to fight against the browser APIs which gets a bit messy.
3. Ionic Vue supports router.go, meaning you could jump from view 10 to view 2. As a result, we need to preserve the history here so we can traverse it without adding new entries. The "last" item in the location history is not necessarily the "current" item because of this. See https://github.com/ionic-team/ionic-framework/blob/main/packages/vue-router/src/locationHistory.ts#L125-L133 for more context.

Definitely agree that the routing files could do with more commenting, though I'm not sure a refactor would solve much. The routing integrations are pretty robust, but it's mainly these edge cases that we are still trying to work through.

I'll update here/on the issue thread when I have a fix for folks to test. Thanks!

[tigohenryschultz]

I took a quick look and noticed that the routeInfo object when clicking the back button in Step 7 in #23873 (comment) shows that the current path name is "/page2" and the previous route name is "/page2". So in other words, it is trying to go back to the same page again. I need to do a deeper dive on why that is getting set.

Regarding location history, the location history is our way of keeping track of navigation items separate from the browser. There are a few reasons for this:

1. The browser does not expose the browser history for (good) security reasons. For context, we only store information regarding route information within the Ionic app. No personal information is stored and this information is not persisted when you close/reload the page.
2. The browser navigation is linear whereas tabs navigation in mobile apps is not linear. So sometimes we need to fight against the browser APIs which gets a bit messy.
3. Ionic Vue supports router.go, meaning you could jump from view 10 to view 2. As a result, we need to preserve the history here so we can traverse it without adding new entries. The "last" item in the location history is not necessarily the "current" item because of this. See https://github.com/ionic-team/ionic-framework/blob/main/packages/vue-router/src/locationHistory.ts#L125-L133 for more context.

Definitely agree that the routing files could do with more commenting, though I'm not sure a refactor would solve much. The routing integrations are pretty robust, but it's mainly these edge cases that we are still trying to work through.

I'll update here/on the issue thread when I have a fix for folks to test. Thanks!

All the classes purposes make sense but as far as how the code is tangled, seems some responsibilities should be moved away/restructured. Or possibly different patterns implemented for readability/less bugs, like fail first, or events.

I'm quite deep in debugging the #23873 (comment) but I am not sure if some of the stuff I am seeing are bugs. Like we were going from our /settings page to /settings/about page and it was trigger this:

viewStacks.mountIntermediaryViews(id, leavingViewItem, delta);

Where it is a one-off for mounting views, and in this case it didn't do anything. The mounting was still performed by:

const setupViewItem = async (matchedRouteRef: any) => {:

Why have two places mount code? Also seems demounting is handled in the viewStacks code(sometimes), why not move mounting there too? I know the mountIntermediaryViews was commented but it still makes no sense.

For the entire block of code:

 if (routerAction === 'replace') {
                    leavingViewItem.mount = false;
                    leavingViewItem.ionPageElement = undefined;
                    leavingViewItem.ionRoute = false;
                }
                else if (!(routerAction === 'push' && routerDirection === 'forward')) {
                    const shouldLeavingViewBeRemoved = routerDirection !== 'none' && leavingViewItem && (enteringViewItem !== leavingViewItem);
                    if (shouldLeavingViewBeRemoved) {
                        leavingViewItem.mount = false;
                        leavingViewItem.ionPageElement = undefined;
                        leavingViewItem.ionRoute = false;
                        viewStacks.unmountLeavingViews(id, enteringViewItem, delta);
                    }
                }
                else {
                    viewStacks.mountIntermediaryViews(id, leavingViewItem, delta);
                }

Why would the leaving view not be mounted, if the user keeps navigating forward, don't demount? Like /settings -> /settings/a -> /settings/a/b? Then if the user starts using the ion-back-button would these be 'demounted'?

Is there a place we can ask code questions?

Why do you inline demount the leaving view but not the enteringViewItem that uses ' viewStacks.unmountLeavingViews', shouldn't the leavingViewItem be passed to 'viewStacks.unmountLeavingViews'? viewStacks.mountEnteringViews does not even exist.

I would imagine the demounting should be performed when the item is removed from the locationHistory stack, not from the IonRouterOutlet code. Now things can get messy on what is mounted/not mounted/in the location stack/not in the location stack.

[liamdebeasi]

setupViewItem is for setting up the view for a new route that we have not seen yet. This is for components that are not yet in the DOM and not yet registered with Vue. This function creates the view item, mounts it, and then proceeds with a transition.

mountIntemediaryViews is primarily for accounting for router.go. When you go from view item 1 to view item 10, you need to remount views 2-9 so that the swipe to go back gesture can occur. unmountLeavingViews is for the same thing, just in the opposite direction (I.e. going from item 10 to item 1). We do not mount the previous views when starting the back transition because you would get a rendering flicker on lower end devices and the view would appear to "pop in".

---

I can understand the desire to get these lingering questions answered, but I'd prefer the focus of this thread be kept on the PR and the code changes contained within the PR. For other general questions about Ionic, you can ask them on the Ionic Forums.

[tigohenryschultz]

setupViewItem is for setting up the view for a new route that we have not seen yet. This is for components that are not yet in the DOM and not yet registered with Vue. This function creates the view item, mounts it, and then proceeds with a transition.

mountIntemediaryViews is primarily for accounting for router.go. When you go from view item 1 to view item 10, you need to remount views 2-9 so that the swipe to go back gesture can occur. unmountLeavingViews is for the same thing, just in the opposite direction (I.e. going from item 10 to item 1). We do not mount the previous views when starting the back transition because you would get a rendering flicker on lower end devices and the view would appear to "pop in".

I can understand the desire to get these lingering questions answered, but I'd prefer the focus of this thread be kept on the PR and the code changes contained within the PR. For other general questions about Ionic, you can ask them on the Ionic Forums.

My only questions would be code specific, which it looks like you are responsible for 99% of. Perhaps I can make a pull request and ask questions/add your answers as comments to commit to the codebase?

setupViewItem should still just create the view item and viewStacks should mount it, then setupViewItem can continue with the transition. There shouldn't be mounting code in 10 places.

[yoyo930021]

Ha, I want to know what after in PR's? Continue to add tests? Or trash?

I found at least four problems in the router.

1. bug: routing breaks after specific sequence of navigations and clicking on ion-back-button #23873

The going-back will break routing

2. bug: vue, router-outlet not waiting for previous transition to finish #24226
3. The first problem in bug: ion-page render problem #24377

The screen will be blank when the quick page switches.

4. The second problem in bug: ion-page render problem #24377

When push new page and go back after, replace page will render the wrong page.

Is the fourth problem the same as the second problem? or the same as the third problem?
Maybe we mix different bugs in the same case?

This PR only can solve the fourth problem.

[mhartington]

Stepping in here.
I'd like to remind @tigohenryschultz that Ionic has a code of conduct.

https://github.com/ionic-team/ionic-framework/blob/main/CODE_OF_CONDUCT.md#our-standards

We expect everyone involved to be professional and respectful.

Locking this PR for now.

EDIT: I had incorrectly assumed bad intent from @yoyo930021. That was a mistake on my part. I have updated the comment to remove their name.

[sean-perkins]

Closing this in favor of PR #24433. Appreciate the time that everyone has put into this and look forward to constructive collaboration on that PR and future works. Thanks all & happy holidays 🎉