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.
…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
Co-authored-by: Shane <shane.king@outsystems.com>
Co-authored-by: ShaneK <shane@shanessite.net>
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
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.
ShaneK
left a comment
There was a problem hiding this comment.
Really nice work getting this to where it is, the ownership model is looking great. Just two issues that might be worth fixing left and they're both my fault. The fullPath stamp I suggested only ever covered the params half, so the delta still has the same-target hole, and the onError I asked for silences vue-router's own logging for any app that hasn't added a handler. I built and tested a patch for each so the comments have a suggested fix if you'd like to take them. That's up to you, though, and if you think these issues are worth addressing. I'm going to approve this now either way, sorry it's taken so long ☠️
| const deltaIsForThisNavigation = | ||
| currentNavigationInfo.to === undefined || | ||
| currentNavigationInfo.to === to.fullPath; |
There was a problem hiding this comment.
The fullPath stamp was my suggestion and I only moved half of it across. Params match on the navigation now, but the delta still goes off the path, so the same-target problem is still open on this side.
Going /home to /list to /detail, then a push('/list') held in a guard and a browser back to /list: the cancelled push reports first and its path matches what's staged, so the back loses its delta. Main gives pop/back with /detail popped, this branch gives push/forward and /detail stays mounted.
I built and tested this one, it fixes the race and keeps all 27 green. Our history listener runs before vue-router's own setupListeners, so the same beforeEach can claim the delta, and the to field comes back off NavigationInformation. It touches six spots so it isn't a suggestion block, sorry:
- to: undefined,
+ };
+ let currentNavigationInfoOwner: RouteLocationNormalized | undefined;
+ let currentNavigationInfoUnclaimed = false;
+
+ const clearNavigationInfo = () => {
+ currentNavigationInfo = {
+ direction: undefined,
+ action: undefined,
+ delta: undefined,
+ };
+ currentNavigationInfoOwner = undefined;
+ currentNavigationInfoUnclaimed = false;
}; router.beforeEach((to: RouteLocationNormalized) => {
if (incomingRouteParamsUnclaimed) { ... }
+
+ if (currentNavigationInfoUnclaimed) {
+ currentNavigationInfoOwner = to;
+ currentNavigationInfoUnclaimed = false;
+ }
}); const deltaIsForThisNavigation =
- currentNavigationInfo.to === undefined ||
- currentNavigationInfo.to === to.fullPath;
+ currentNavigationInfoOwner === undefined
+ ? currentNavigationInfoUnclaimed
+ : currentNavigationInfoOwner === to;- opts.history.listen((to: any, _x: any, info: any) => {
+ opts.history.listen((_to: any, _x: any, info: any) => {
direction: info.direction === "" ? "forward" : info.direction,
-
- to,
};
+ currentNavigationInfoOwner = undefined;
+ currentNavigationInfoUnclaimed = true;
});Both currentNavigationInfo = { ... } resets become clearNavigationInfo(), and the to field comes out of NavigationInformation.
One honest caveat: an async component leave guard could let another navigation claim the delta before the popstate one does, the same shape as the params fallback. Narrower than the path compare it replaces, but not zero.
A regression test for it, written to match the racing tests already in the file. This fails on the branch as it is and passes with the change:
// Guards against clearing a delta that belongs to another navigation still in flight.
it('should keep the delta of a back navigation that a cancelled push shares a path with', async () => {
// /home -> /list -> /detail, then a held push('/list') racing a back to /list.
// Release the push first so it reports cancelled before the back settles.
holding = true;
router.push('/list');
await waitUntil(() => holds.length === 1, 'the push to reach the guard');
router.back();
await waitUntil(() => holds.length === 2, 'the back to reach the guard');
holds[0]();
await pushCancelled;
holds[1]();
await waitForRouter();
expect(currentRoute(navManager)).toEqual({
pathname: '/list',
routerAction: 'pop',
routerDirection: 'back'
});
expect(viewStack(wrapper)).toEqual([
{ id: 'home', hidden: true },
{ id: 'list', hidden: false }
]);
});| /** | ||
| * A guard that throws, including an await on a session check that rejects, | ||
| * never reaches afterEach. vue-router rejects the navigation promise | ||
| * instead, so there is no failure to inspect there and the staged state | ||
| * would survive. This does not handle the error, so navigation outcomes are | ||
| * unchanged. | ||
| * | ||
| * A guard that returns a location is still not covered, because that | ||
| * redirects rather than fails and afterEach is never called for the original | ||
| * navigation. | ||
| */ | ||
| router.onError((_error: unknown, to: RouteLocationNormalized) => { | ||
| discardStagedStateFor(to); | ||
| }); |
There was a problem hiding this comment.
| /** | |
| * A guard that throws, including an await on a session check that rejects, | |
| * never reaches afterEach. vue-router rejects the navigation promise | |
| * instead, so there is no failure to inspect there and the staged state | |
| * would survive. This does not handle the error, so navigation outcomes are | |
| * unchanged. | |
| * | |
| * A guard that returns a location is still not covered, because that | |
| * redirects rather than fails and afterEach is never called for the original | |
| * navigation. | |
| */ | |
| router.onError((_error: unknown, to: RouteLocationNormalized) => { | |
| discardStagedStateFor(to); | |
| }); | |
| /** | |
| * vue-router only logs an uncaught navigation error while no error handler | |
| * is registered, so the handler below would silence it for every app that | |
| * has not added one of its own. Track whether the app adds one so the log | |
| * can be put back when it has not. | |
| */ | |
| let appErrorHandlers = 0; | |
| const addErrorHandler = router.onError.bind(router); | |
| router.onError = (handler) => { | |
| appErrorHandlers++; | |
| const removeHandler = addErrorHandler(handler); | |
| return () => { | |
| appErrorHandlers--; | |
| removeHandler(); | |
| }; | |
| }; | |
| /** | |
| * A guard that throws, including an await on a session check that rejects, | |
| * never reaches afterEach. vue-router rejects the navigation promise | |
| * instead, so there is no failure to inspect there and the staged state | |
| * would survive. The error is re-thrown to the app the same way it was | |
| * before, so navigation outcomes are unchanged. | |
| * | |
| * A guard that returns a location is still not covered, because that | |
| * redirects rather than fails and afterEach is never called for the original | |
| * navigation. | |
| */ | |
| addErrorHandler((error: unknown, to: RouteLocationNormalized) => { | |
| discardStagedStateFor(to); | |
| if (appErrorHandlers === 0) { | |
| console.error(error); | |
| } | |
| }); |
Asking for onError was my idea and I missed that registering a handler changes vue-router's own behaviour. Internally it only logs while errorListeners is empty, so ours kills that branch for any app that hasn't added its own. With a guard that throws, plain vue-router logs an error and a dev warning, and so does main, but this branch logs nothing. The rejected promise is all that's left, and the popstate path swallows that itself, so a throwing guard during a browser back is silent.
The suggestion counts the app's handlers and puts the log back when there are none. I tested it: no handler gets one log, an app handler gets one call and no duplicate log, and unregistering restores the log. We already reassign install and isReady over in index.ts, so patching onError is in keeping. This only brings back console.error, not the dev-only diagnostic, since that one would mean copying vue-router's own message.
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