Repository navigation
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>
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.
Co-Authored-By: Shane <561207+ShaneK@users.noreply.github.com>
Co-Authored-By: Shane <561207+ShaneK@users.noreply.github.com>
…onic-team#31510) Issue number: internal --------- ## What is the current behavior? Currently, when a guard returns a location instead of `false` during a back navigation, the URL updates to the redirect target but Ionic renders a different page. A redirect doesn't fail, so the original navigation never reaches `afterEach` or `onError`. Its staged delta and params are never cleared and get applied to the redirect target instead. ## What is the new behavior? The ownership `beforeEach` now discards the state a navigation claimed when a guard redirects it. We match it through `redirectedFrom`, which vue-router points at the first location in the redirect chain. That covers leave guards, which run before our hook, and a guard redirect to the current page, which fails as a duplicate before reaching `beforeEach`. ## Does this introduce a breaking change? - [ ] Yes - [X] No ## Other information This was [raised while reviewing ionic-team#31364](ionic-team#31364 (comment)) and left out of scope there.
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