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
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.
…tion Co-authored-by: ShaneK <561207+ShaneK@users.noreply.github.com>
ShaneK
left a comment
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
Co-authored-by: Shane <shane.king@outsystems.com>
Co-authored-by: ShaneK <shane@shanessite.net>
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
Really nice work on this, and I think the stamp was the right call. Two things worry me though. The stamp only got wired into setIncomingRouteParams, so changeTab and handleNavigateBack leave a stale one behind and the gate ends up clearing the wrong navigation's params. And the branch the stamp exists for doesn't have a test. The rest is a couple of inputs the gate can't match, which I don't think need solving here, just naming. I answered your question about the separate variable up in that thread.
| to: undefined, | ||
| }; | ||
|
|
||
| incomingRouteParams = undefined; |
There was a problem hiding this comment.
Nah, keep it as it is. Your reason holds, and I think there's a better version of it. Since handleNavigateBack re-stages a stored RouteInfo as params, a leaked to wouldn't just end up on a RouteInfo, it'd come back out of locationHistory later as a stale target and give you a confidently wrong match at the gate. That's worse than cosmetic.
The shape isn't really what's biting you though, and I left a comment on the gate about that. Folding to onto the object wouldn't fix changeTab either, since it spreads ...incomingRouteParams and would carry the old target forward just the same. What fixes it is having one place that writes the params, so make setIncomingRouteParams the only writer and give the other two a way through it. You'd still have two variables, but only one spot that can desync them, which is the guarantee the single object would've bought you.
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
Really nice work on this so far! There are two more cancellation cases where the newer navigation still loses its params, plus a couple of test follow-ups.
| const paramsAreForThisNavigation = | ||
| incomingRouteParamsTo === undefined | ||
| ? deltaIsForThisNavigation | ||
| : incomingRouteParamsTo === to.fullPath || |
There was a problem hiding this comment.
The fullPath stamp was my suggestion, but I missed same-target navigations. A held push to /login, followed by replace/root to the same path, still comes out replace/none. Home stays mounted and canGoBack() stays true. Could we give each staged navigation its own identity?
| const prevInfo = locationHistory.findLastLocation(routeInfo); | ||
| if (prevInfo) { | ||
| incomingRouteParams = { | ||
| stageRouteParams({ |
There was a problem hiding this comment.
I suggested leaving this target undefined to fall back to the delta, but I missed the non-linear handleNavigateBack() branch that uses router.replace() without one. If the older push reports cancellation first, the back comes out replace/none instead of pop/back. Could this branch stamp the replace target?
There was a problem hiding this comment.
Changed the approach rather than stamping this branch: params are now matched on the navigation that owns them instead of where it was heading, so this branch stages unclaimed and its replace claims ownership in beforeEach like anything else. No target or delta needed here any more.
Couldn't get a spec onto this branch though. Nothing in the suite reaches it, and every sequence I tried came out with a negative positionDelta and took router.go or the linear back. Let me know if you know the state that gets there.
| router.push('/settings'); | ||
| await waitForRouter(); | ||
|
|
||
| expect(currentRoute(navManager)).toEqual({ |
There was a problem hiding this comment.
Looks like this one got missed from the stack assertion change. The viewStack() check passes with Home and Profile hidden and Settings visible, would you mind adding it here too?
| * which cost the tab its direction and its tab name. | ||
| */ | ||
| navManager.handleNavigate('/details', 'push', 'forward'); | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); |
There was a problem hiding this comment.
Could this use promise gates like the back-navigation test below? They cover the same ordering without the fixed 50ms window or 1.2s wait. Just a test cleanup though, no worries if you don't want to change it here.
2 participants
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:
9.0.1-dev.11788285010.17561004