Skip to content

Commit

Permalink
fix(vue): routing history is correctly replaced when overwriting brow…
Browse files Browse the repository at this point in the history 
…ser history (#24670)

resolves #23873
  • Loading branch information
@liamdebeasi
liamdebeasi committed Feb 2, 2022
1 parent bf9b4df commit 0b18260
Show file tree
Hide file tree
Showing 6 changed files with 341 additions and 145 deletions.
33 changes: 18 additions & 15 deletions packages/vue-router/src/locationHistory.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,25 @@ export const createLocationHistory = () => {
locationHistory.push(routeInfo);
}

const clearHistory = () => {
locationHistory.length = 0;
/**
* Wipes the location history arrays.
* You can optionally provide a routeInfo
* object which will wipe that entry
* and every entry that appears after it.
*/
const clearHistory = (routeInfo?: RouteInfo) => {
Object.keys(tabsHistory).forEach(key => {
tabsHistory[key] = [];
});

if (routeInfo) {
const existingRouteIndex = locationHistory.findIndex(r => r.position === routeInfo.position);
if (existingRouteIndex === -1) return;

locationHistory.splice(existingRouteIndex);
} else {
locationHistory.length = 0;
}
}
const getTabsHistory = (tab: string): RouteInfo[] => {
let history;
Expand All @@ -105,17 +119,6 @@ export const createLocationHistory = () => {

const size = () => locationHistory.length;

const updateByHistoryPosition = (routeInfo: RouteInfo, updateEntries: boolean) => {
const existingRouteIndex = locationHistory.findIndex(r => r.position === routeInfo.position);
if (existingRouteIndex === -1) return;

locationHistory[existingRouteIndex].pathname = routeInfo.pathname;

if (updateEntries) {
locationHistory[existingRouteIndex].pushedByRoute = routeInfo.pushedByRoute;
}
}

/**
* Finds and returns the location history item
* given the state of browser's history API.
Expand Down Expand Up @@ -205,7 +208,6 @@ export const createLocationHistory = () => {

return {
current,
updateByHistoryPosition,
size,
last,
previous,
Expand All @@ -214,6 +216,7 @@ export const createLocationHistory = () => {
update,
getFirstRouteInfoForTab,
getCurrentRouteInfoForTab,
findLastLocation
findLastLocation,
clearHistory
}
}
98 changes: 82 additions & 16 deletions packages/vue-router/src/router.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export const createIonRouter = (opts: IonicVueRouterOptions, router: Router) =>
* new pages or updating history via the forward
* and back browser buttons.
*/
const initialHistoryPosition = opts.history.state.position as number;
let initialHistoryPosition = opts.history.state.position as number;
let currentHistoryPosition = opts.history.state.position as number;

let currentRouteInfo: RouteInfo;
Expand Down Expand Up @@ -159,8 +159,60 @@ export const createIonRouter = (opts: IonicVueRouterOptions, router: Router) =>
leavingLocationInfo = locationHistory.previous();
} else if (incomingRouteParams.routerAction === 'pop') {
leavingLocationInfo = locationHistory.current(initialHistoryPosition, currentHistoryPosition + 1);

/**
* If the Ionic Router action was "pop"
* and the browser history action was "replace", then
* it is the case that the user clicked an IonBackButton
* that is trying to go back to the route specified
* by the defaultHref property.
*
* The problem is that this route currently does
* not exist in the browser history, and we cannot
* prepend an item in the browser's history stack.
* To work around this, we replace the state of
* the current item instead.
* Given this scenario:
* /page2 --> /page3 --> (back) /page2 --> (defaultHref) /page1
* We would replace the state of /page2 with the state of /page1.
*
* When doing this, we are essentially re-writing past
* history which makes the future history no longer relevant.
* As a result, we clear out the location history so that users
* can begin pushing new routes to the stack.
*
* This pattern is aligned with how the browser handles
* pushing new routes after going back as well as how
* other stack based operations such as undo/redo work.
* For example, if you do tasks A, B, C, undo B and C, and
* then do task D, you cannot "redo" B and C because you
* rewrote the stack's past history.
*
* With browser history, it is a similar concept.
* Going /page1 --> /page2 --> /page3 and then doing
* router.go(-2) will bring you back to /page1.
* If you then push /page4, you have rewritten
* the past history and you can no longer go
* forward to /page2 or /page3.
*/
if (action === 'replace') {
locationHistory.clearHistory();
}
} else {
leavingLocationInfo = locationHistory.current(initialHistoryPosition, currentHistoryPosition - 1);
/**
* If the routerDirection was specified as "root", then
* we are replacing the initial state of location history
* with this incoming route. As a result, the leaving
* history info is stored at the same location as
* where the incoming history location will be stored.
*
* Otherwise, we can assume this is just another route
* that will be pushed onto the end of location history,
* so we can grab the previous item in history relative
* to where the history state currently is.
*/
const position = (incomingRouteParams.routerDirection === 'root') ? currentHistoryPosition : currentHistoryPosition - 1;
leavingLocationInfo = locationHistory.current(initialHistoryPosition, position);
}
} else {
leavingLocationInfo = currentRouteInfo;
Expand Down Expand Up @@ -285,26 +337,40 @@ export const createIonRouter = (opts: IonicVueRouterOptions, router: Router) =>
*/
if (historySize > historyDiff && routeInfo.tab === undefined) {
/**
* When going from /a --> /a/1 --> /b, then going
* back to /a, then going /a --> /a/2 --> /b, clicking
* the ion-back-button should return us to /a/2, not /a/1.
* However, since the route entry for /b already exists,
* we need to update other information such as the "pushedByRoute"
* so we know which route pushed this new route.
* When navigating back through the history,
* if users then push a new route the future
* history stack is no longer relevant. As
* a result, we need to clear out all entries
* that appear after the current routeInfo
* so that we can then append the new history.
*
* This does not apply when using router.go
* as that is traversing through the history,
* not altering it.
*
* However, when using router.go with a stride of >1 or <-1,
* we should not update this additional information because
* we are traversing through the history, not pushing new states.
* Going from /a --> /b --> /c, then doing router.go(-2), then doing
* router.go(2) to go from /a --> /c should not update the route
* listing to say that /c was pushed by /a.
* Previously we had only updated the existing route
* and then left the future history alone. That
* worked for some use cases but was not sufficient
* in other scenarios.
*/
const hasDeltaStride = delta !== undefined && Math.abs(delta) !== 1;
locationHistory.updateByHistoryPosition(routeInfo, !hasDeltaStride);

if (routeInfo.routerAction === 'push' && delta === undefined) {
locationHistory.clearHistory(routeInfo);
locationHistory.add(routeInfo);
}
} else {
locationHistory.add(routeInfo);
}

/**
* If we recently reset the location history
* then we also need to update the initial
* history position.
*/
if (locationHistory.size() === 1) {
initialHistoryPosition = routeInfo.position;
}

currentRouteInfo = routeInfo;
}
incomingRouteParams = undefined;
Expand Down
5 changes: 4 additions & 1 deletion packages/vue/test-app/src/App.vue
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,17 @@
</template>

<script lang="ts">
import { IonApp, IonRouterOutlet } from '@ionic/vue';
import { IonApp, IonRouterOutlet, useIonRouter } from '@ionic/vue';
import { defineComponent } from 'vue';
export default defineComponent({
name: 'App',
components: {
IonApp,
IonRouterOutlet
},
setup() {
(window as any).debugIonRouter = useIonRouter();
}
});
</script>
113 changes: 113 additions & 0 deletions packages/vue/test-app/tests/e2e/specs/routing.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,119 @@ describe('Routing', () => {
cy.ionPageVisible('home');
cy.ionPageHidden('inputs');
})

// Verifies fix for https://github.com/ionic-team/ionic-framework/issues/23873
it('should correctly show pages after going back to defaultHref page', () => {
cy.visit('http://localhost:8080/default-href');

cy.routerPush('/routing');
cy.ionPageVisible('routing');
cy.ionPageHidden('defaulthref');

cy.ionBackClick('routing');
cy.ionPageDoesNotExist('routing');
cy.ionPageVisible('defaulthref');

cy.ionBackClick('defaulthref');
cy.ionPageDoesNotExist('defaulthref');
cy.ionPageVisible('home');

cy.routerPush('/default-href');
cy.ionPageVisible('defaulthref');
cy.ionPageHidden('home');

cy.routerPush('/routing');
cy.ionPageVisible('routing');
cy.ionPageHidden('defaulthref');

cy.ionBackClick('routing');
cy.ionPageDoesNotExist('routing');
cy.ionPageVisible('defaulthref');

cy.ionBackClick('defaulthref');
cy.ionPageDoesNotExist('defaulthref');
cy.ionPageVisible('home');
})

it('should correctly update location history after rewriting past state', () => {
cy.visit('http://localhost:8080');

cy.routerPush('/routing');
cy.ionPageVisible('routing');
cy.ionPageHidden('home');

cy.routerPush('/inputs');
cy.ionPageVisible('inputs');
cy.ionPageHidden('routing');

cy.ionBackClick('inputs');
cy.ionPageVisible('routing');
cy.ionPageDoesNotExist('inputs');

cy.ionBackClick('routing');
cy.ionPageVisible('home');
cy.ionPageDoesNotExist('routing');

cy.routerPush('default-href');
cy.ionPageVisible('defaulthref');
cy.ionPageHidden('home');

cy.routerPush('/routing');
cy.ionPageVisible('routing');
cy.ionPageHidden('defaulthref');

cy.routerPush('/inputs');
cy.ionPageVisible('inputs');
cy.ionPageHidden('routing');

cy.ionBackClick('inputs');
cy.ionPageVisible('routing');
cy.ionPageDoesNotExist('inputs');

cy.ionBackClick('routing');
cy.ionPageVisible('defaulthref');
cy.ionPageDoesNotExist('routing');
})

it('should correctly update location history after setting root state', () => {
cy.visit('http://localhost:8080');

cy.routerPush('/routing');
cy.ionPageVisible('routing');
cy.ionPageHidden('home');

cy.routerPush('/routing/1');
cy.ionPageVisible('routingparameter-1');
cy.ionPageHidden('routing');

cy.ionBackClick('routingparameter-1');
cy.ionPageVisible('routing');
cy.ionPageDoesNotExist('routingparameter-1');

cy.ionBackClick('routing');
cy.ionPageVisible('home');
cy.ionPageDoesNotExist('routing');

cy.ionRouterNavigate('/inputs', 'root')
cy.ionPageVisible('inputs');
cy.ionPageDoesNotExist('home');

cy.routerPush('/routing');
cy.ionPageVisible('routing');
cy.ionPageHidden('inputs');

cy.routerPush('/routing/1');
cy.ionPageVisible('routingparameter-1');
cy.ionPageHidden('routing');

cy.ionBackClick('routingparameter-1');
cy.ionPageVisible('routing');
cy.ionPageDoesNotExist('routingparameter-1');

cy.ionBackClick('routing');
cy.ionPageVisible('inputs');
cy.ionPageDoesNotExist('routing');
})
});

describe('Routing - Swipe to Go Back', () => {
Expand Down
6 changes: 6 additions & 0 deletions packages/vue/test-app/tests/e2e/support/commands.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,12 @@ Cypress.Commands.add('routerGo', (n) => {
});
});

Cypress.Commands.add('ionRouterNavigate', (...args) => {
cy.window().then(win => {
win.debugIonRouter.navigate(...args);
});
});

Cypress.Commands.add('ionBackButtonHidden', (pageId) => {
cy.get(`div.ion-page[data-pageid=${pageId}]`)
.should('be.visible', true)
Expand Down

0 comments on commit 0b18260

Please sign in to comment.