Conversation
Co-authored-by: zhiqiang.guo <zguoby@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ShaneK
left a comment
There was a problem hiding this comment.
Nice find on the cancelled issue, matching vue-router's own revert gate is the right call. I think incomingRouteParams needs clearing alongside currentNavigationInfo though, otherwise the ion-back-button path still breaks. A couple of smaller notes on the test as well.
| currentNavigationInfo = { | ||
| direction: undefined, | ||
| action: undefined, | ||
| delta: undefined, | ||
| }; |
There was a problem hiding this comment.
| currentNavigationInfo = { | |
| direction: undefined, | |
| action: undefined, | |
| delta: undefined, | |
| }; | |
| currentNavigationInfo = { | |
| direction: undefined, | |
| action: undefined, | |
| delta: undefined, | |
| }; | |
| incomingRouteParams = undefined; |
I think currentNavigationInfo is only half the staged state here. Calling handleNavigateBack() also sets incomingRouteParams before it calls router.back(), so an aborted back leaves that behind too, still holding the previous route's id.
Tapping ion-back-button on /profile with the guard returning false, then pushing /settings, leaves you on /settings while Ionic still reports /home with a pop action, and canGoBack() goes false so the back button disappears. Adding the clear above fixes it without changing any of the other guard-failure cases I tried.
| * history entry, so its info is still accurate and stays in place for | ||
| * the superseding navigation to consume. | ||
| */ | ||
| if (!isNavigationFailure(failure, NavigationFailureType.cancelled)) { |
There was a problem hiding this comment.
This only runs for failures that actually reach afterEach. If a guard diverts a back navigation by returning a location instead of false, which is the usual auth-guard shape, vue-router doesn't revert the history entry and never calls afterEach for the original navigation at all.
A guard that returns /login on a back from /profile puts you on /login while Ionic still reports /home with a pop and delta: -1, same as before this change. The filed issue only uses return false so I don't think it needs solving here, but the way the comment is worded makes it sound like every diverted navigation is handled.
There was a problem hiding this comment.
Follow up ticket (FW-7699) has been created: 7ae3b16
| }); | ||
|
|
||
| // Verifies fix for https://github.com/ionic-team/ionic-framework/issues/29721 | ||
| it('should keep the view stack intact after a navigation guard blocks going back', async () => { |
There was a problem hiding this comment.
The cancelled branch has no coverage here, even though it's the subtle half of the change. These assertions also check mount and destroy but not direction, so a fix that produced routerDirection: 'none' instead of 'forward' would still pass.
The title's a bit off too, since asserting right after the blocked back() is green without the fix either way. The third assertion after the push to /home is the one that catches the regression, so maybe name it after that. Up to you though.
There was a problem hiding this comment.
You were right that this was the subtle half. Writing the coverage showed the exclusion was wrong, so I removed it.
The two afterEach calls can land in either order. When the push completes first, keeping and clearing are identical. When the cancellation is reported first, keeping the state means you push /settings and land on Home with Settings never mounted, which is this PR's bug arriving through the cancelled path.
The hole in the reasoning we both agreed on: the revert gate tells us the history entry is still valid, but the staged state describes the navigation that was cancelled, and the one reading it next is the one that replaced it.
| const createPage = (id: string) => ({ | ||
| components: { IonPage }, | ||
| name: id, | ||
| template: `<ion-page data-page="${id}"></ion-page>` |
There was a problem hiding this comment.
Could this be data-pageid? That's what BasePage in this file uses, along with router-outlet.spec.ts and most of the app views, so data-page makes a third spelling of the same thing. The viewStack() helper reads it too, so that would have to change with it.
Keeping the local factory looks like the right call though, since BasePage's :data-pageid="name" doesn't actually render anything in Vue 3.
| const router = createRouter({ | ||
| history: createWebHistory(process.env.BASE_URL), | ||
| routes: [ | ||
| { path: '/', redirect: '/home' }, |
There was a problem hiding this comment.
Did you mean for the test to enter through this with router.push('/')? Right now it pushes /home directly so the route never gets used, and the other tests here that declare the redirect do go through it.
| { id: 'profile', hidden: true } | ||
| ]); | ||
|
|
||
| isLoggedIn = true; |
There was a problem hiding this comment.
Was there meant to be another assertion after this? It doesn't affect anything as written, since the guard only fires when from.path is /profile and nothing runs after this navigation. It reads like setup that matters, so I went looking for what it did.
…tion Co-authored-by: ShaneK <561207+ShaneK@users.noreply.github.com>
ShaneK
left a comment
There was a problem hiding this comment.
Really nice work on this so far!
Just one thing I'd definitely like to be worked out before we can merge this, which is that dropping the carve-out regresses the opposite ordering, where another back replaces the cancelled one. The rest is mostly nits
| currentNavigationInfo = { | ||
| direction: undefined, | ||
| action: undefined, | ||
| delta: undefined, | ||
| }; | ||
|
|
||
| incomingRouteParams = undefined; |
There was a problem hiding this comment.
Good catch on this one, that ordering was genuinely broken and I'd called it safe. I think dropping the carve-out breaks the opposite case though. When another back replaces the cancelled navigation, that second back has already staged its own info by the time this runs, so this clears its delta instead.
Two quick backs and the second comes out as push/forward with a new routeInfo id, so the page rebuilds instead of restoring and the back button stops working there. All three were fine on the last commit.
I think you could keep both if you saved the target path alongside the delta in history.listen, then skipped the clear when it doesn't match the failed navigation. I tried that and everything passed, including your new test.
There was a problem hiding this comment.
Confirmed, and reproduced it before changing anything. With the unconditional clear the second back comes out as replace/none with a new routeInfo id and nothing unmounted.
So we each found a real bug pointing opposite ways. The difference is that a second back stages its own info through history.listen before the clear runs, so the clear was wiping the live note rather than the stale one.
Took your suggestion. history.listen now records the location it moved to, and the clear only runs when that matches the failed navigation. Kept the undefined case so programmatic navigations still clear their incomingRouteParams.
Added should keep the delta of a back navigation that replaced a cancelled one, which holds both backs in their guards rather than relying on timing. All three cases pass together now.
| if (failure) return; | ||
| if (failure) { | ||
| /* | ||
| * vue-router reverts the history entry for aborted and duplicated |
There was a problem hiding this comment.
Couple of things in here read a bit off. The revert only happens for popstate navigations - a failed push or replace never writes its entry in the first place, so there's nothing to revert.
And handleNavigateBack isn't the only thing staging params - setIncomingRouteParams does it too for goBack, push, replace and tab changes, and it doesn't set an id, so the previous route's id bit only applies to the back button.
| }); | ||
|
|
||
| // Verifies fix for https://github.com/ionic-team/ionic-framework/issues/29721 | ||
| it('should keep canGoBack accurate after a guard blocks a back button navigation', async () => { |
There was a problem hiding this comment.
Is the back button the right thing to name here? Calling ionRouter.back() goes through goBack, but the back button goes through handleNavigateBack, which stages the whole previous route including its id. That id is what skips the pathname overwrite, so it's the worse of the two and the one I meant originally. Your fix does cover it, there's just nothing testing it.
| * can be read without wrapping the outlet in another component. | ||
| */ | ||
| const currentRoute = () => { | ||
| const routeInfo = wrapper.vm.$.appContext.provides.navManager.getCurrentRouteInfo(); |
There was a problem hiding this comment.
Could this use a wrapper that injects navManager instead? Reading wrapper.vm.$.appContext.provides digs into Vue's internal instance, and these two are the only places in the package doing that, so it could break quietly on a Vue minor. The AppWithInject shape in your other new test does the same job through inject, which is how useIonRouter gets it too.
|
|
||
| // Verifies fix for https://github.com/ionic-team/ionic-framework/issues/29721 | ||
| it('should keep the previous page when pushing after a guard blocks going back', async () => { | ||
| const createPage = (id: string) => ({ |
There was a problem hiding this comment.
Do you think this is worth hoisting next to BasePage? It's the same in all three new tests, and the page fixture is the one thing this file already keeps at module scope. Still right to keep it separate from BasePage, for the reason here.
Co-authored-by: Shane <shane.king@outsystems.com>
ShaneK
left a comment
There was a problem hiding this comment.
Really nice work on this! One thing worries me a bit is that the path-matching gate I suggested only scopes the delta, not incomingRouteParams, and that regresses a logout replace against main. My fault for not spotting it when I proposed it. The rest is nits.
| to: undefined, | ||
| }; | ||
|
|
||
| incomingRouteParams = undefined; |
There was a problem hiding this comment.
The gate here was my suggestion, and I missed something when I proposed it. Saving the target path works for the delta, but this line clears incomingRouteParams too, and nothing ever stamps a path onto that. Only history.listen writes currentNavigationInfo.to, and neither setIncomingRouteParams nor changeTab records one, so whenever neither navigation is a history traversal the first disjunct is true and the clear runs unconditionally.
That regresses against main. With a push in flight on a lazy route, a logout ionRouter.replace('/login') cancels it, and the cancelled push wipes the replace's staged params before it gets to use them. The route comes out routerDirection: 'none' with canGoBack() true, where main gives 'root' and false. Without 'root' there's no clearHistory(), so the back button on the login page still walks into the authenticated pages after logout. The tab path loses tab and routerAnimation the same way.
I think stamping the resolved path onto incomingRouteParams, the way history.listen already stamps currentNavigationInfo.to, is the fix that keeps the gate honest for both slots. I haven't tried that one. What I did try is skipping just the params clear on cancelled failures, which gets main's result back with all 20 tests still passing, since 29721's own repro aborts rather than cancels. That leaves an ionRouter.back() cancelled by a push still uncleared though, which is why I'd lean toward the stamp.
There was a problem hiding this comment.
Went with the stamp. setIncomingRouteParams now takes an optional target and resolves it, and the gate is two independent checks instead of one answer for both slots. goBack and goForward leave it unset and fall back to the delta's target, which is fine because those are the ones that hand off to history.
One thing I'd like your read on. I kept the target in a separate incomingRouteParamsTo rather than adding a to field to incomingRouteParams. The reason is that the params get spread wholesale onto a RouteInfo at the incomingRouteParams?.id branch, so a field on them would land there too and reach anything reading route info. The cost is two variables to keep in sync, and I had to clear the stamp in all three places the params are cleared. Happy to move it onto the params if you'd rather have one object, since the leak is cosmetic rather than harmful.
| let releaseFirstBack: () => void; | ||
| let releaseSecondBack: () => void; |
There was a problem hiding this comment.
| let releaseFirstBack: () => void; | |
| let releaseSecondBack: () => void; | |
| let releaseFirstBack!: () => void; | |
| let releaseSecondBack!: () => void; |
Looks like the ! from the earlier thread only made it onto the other racing test. Same reason here, tsc reports TS2454 on both since they're assigned in the guard's promise executors but called from the it body. The two below are fine as they are, nothing calls those outside a callback.
| ) => { | ||
| if (failure) return; | ||
| if (failure) { | ||
| /* |
There was a problem hiding this comment.
Could this be a /** */ block? There isn't a plain one anywhere in this package today, and the comment you added on the history.listen callback already uses it. Tiny thing, up to you.
| expect(routeInfo.routerDirection).toEqual('forward'); | ||
| }); | ||
|
|
||
| // Verifies fix for https://github.com/ionic-team/ionic-framework/issues/29721 |
There was a problem hiding this comment.
This one passes on main. Pre-fix nothing gets cleared, so there's no over-clearing for it to catch, which makes it a guard against a regression this change could introduce rather than a repro of 29721. I'd drop the issue link here and leave it on the other four.
| router.push('/settings'); | ||
| await waitForRouter(); | ||
|
|
||
| const routeInfo = navManager.getCurrentRouteInfo(); |
There was a problem hiding this comment.
Would you mind asserting the stack here too? This test and the last one check pathname, action and direction but never that the right page ended up on screen, and both run through the same view-stack path the bug damaged, so a regression that keeps routeInfo right and drops a page would still pass. The currentRoute() helper from the first test already does the routeInfo half, and hoisting it would cover both. Worth a look at the viewStack() copy in the cancelled-back test as well, since it drops the ion-page-hidden flag that makes it interesting.
Co-authored-by: ShaneK <shane@shanessite.net>
Issue number: resolves #29721
What is the current behavior?
A navigation guard that cancels a back navigation leaves Ionic's staged navigation info behind. The next push reads that stale delta, gets mistaken for history traversal, and the incoming route is never added to the location history. The router outlet then destroys a page it should have kept.
What is the new behavior?
currentNavigationInfois cleared beforerouter.afterEachreturns on a navigation failure.cancelledfailures, matching vue-router, which reverts the history entry forabortedandduplicatednavigations but leaves it in place when a navigation is superseded.Does this introduce a breaking change?
Other information
Dev build:
8.8.19-dev.11787096841.12dc6efd