From f891f667082d2deb5f1b5f0f27af46e46ed1ca0f Mon Sep 17 00:00:00 2001 From: Ely Lucas Date: Tue, 15 Dec 2020 09:03:08 -0700 Subject: [PATCH] fix(react): improve view matching logic (#22569) --- .../src/ReactRouter/IonRouter.tsx | 9 +- packages/react-router/test-app/build.sh | 2 +- .../react-router/test-app/package-lock.json | 12 +- packages/react-router/test-app/package.json | 2 +- packages/react/src/routing/LocationHistory.ts | 30 +-- .../routing/__tests__/LocationHistory.spec.ts | 219 ++++++++++++++++++ packages/react/tsconfig.json | 1 - 7 files changed, 246 insertions(+), 29 deletions(-) create mode 100644 packages/react/src/routing/__tests__/LocationHistory.spec.ts diff --git a/packages/react-router/src/ReactRouter/IonRouter.tsx b/packages/react-router/src/ReactRouter/IonRouter.tsx index a0e8481aa2..7f4023bedb 100644 --- a/packages/react-router/src/ReactRouter/IonRouter.tsx +++ b/packages/react-router/src/ReactRouter/IonRouter.tsx @@ -118,15 +118,14 @@ class IonRouterInner extends React.PureComponent { }; } if (action === 'POP') { - const ri = this.locationHistory.current(); - if (ri && ri.pushedByRoute) { - const prevInfo = this.locationHistory.findLastLocation(ri); + const currentRoute = this.locationHistory.current(); + if (currentRoute && currentRoute.pushedByRoute) { + const prevInfo = this.locationHistory.findLastLocation(currentRoute); this.incomingRouteParams = { ...prevInfo, routeAction: 'pop', routeDirection: 'back' }; } else { - const direction = 'none'; this.incomingRouteParams = { routeAction: 'pop', - routeDirection: direction, + routeDirection: 'none', tab: this.currentTab, }; } diff --git a/packages/react-router/test-app/build.sh b/packages/react-router/test-app/build.sh index 70f48f8a2a..1123c50fde 100755 --- a/packages/react-router/test-app/build.sh +++ b/packages/react-router/test-app/build.sh @@ -3,7 +3,7 @@ cd ../../react npm run build npm pack cd ../react-router/test-app -npm i ../../react/ionic-react-5.3.0.tgz +npm i ../../react/ionic-react-5.5.0.tgz npm run start diff --git a/packages/react-router/test-app/package-lock.json b/packages/react-router/test-app/package-lock.json index 2485ddc5b1..ff40146cca 100644 --- a/packages/react-router/test-app/package-lock.json +++ b/packages/react-router/test-app/package-lock.json @@ -1513,19 +1513,19 @@ } }, "@ionic/core": { - "version": "5.3.2", - "resolved": "https://registry.npmjs.org/@ionic/core/-/core-5.3.2.tgz", - "integrity": "sha512-s/SDnS993fnZ3d6EzOlURHBbc2aI2/WsZSsCLgnJz3G+KUO4hY/2RQvdmtl0FZpDHMSyehG6tRgFFXvnFSI9CQ==", + "version": "5.5.0", + "resolved": "https://registry.npmjs.org/@ionic/core/-/core-5.5.0.tgz", + "integrity": "sha512-LHKRQ4kmJN6Z9aqbYajYIQi3CFULjO/UI16frMu2Oc4rx8uewMNPBClAR5Dhy/jSkL3MiIOiuQzLPO4Kyd/zzw==", "requires": { "ionicons": "^5.1.2", "tslib": "^1.10.0" } }, "@ionic/react": { - "version": "file:../../react/ionic-react-5.3.2.tgz", - "integrity": "sha512-ojxb7wNfikrpREjlZwRhwzLEmHtdNGqQIX73pyV/kZbgKx4z2ay5knSbdguYeIjPMQHUjCCgyog77hONxGf5Rw==", + "version": "file:../../react/ionic-react-5.5.0.tgz", + "integrity": "sha512-/MiNrJbGH9hGPglF5dGxNKSA+XmYFuzOO45DEZ99lOUS0fIXHMfcQ4PSiWEA/n3++qQIgLTHPn3IG4zDaYYpTQ==", "requires": { - "@ionic/core": "5.3.2", + "@ionic/core": "5.5.0", "ionicons": "^5.1.2", "tslib": "*" } diff --git a/packages/react-router/test-app/package.json b/packages/react-router/test-app/package.json index f54882c210..530dffd9de 100644 --- a/packages/react-router/test-app/package.json +++ b/packages/react-router/test-app/package.json @@ -7,7 +7,7 @@ "@capacitor/android": "^2.2.0", "@capacitor/core": "1.5.2", "@capacitor/ios": "^2.2.0", - "@ionic/react": "file:../../react/ionic-react-5.3.2.tgz", + "@ionic/react": "file:../../react/ionic-react-5.5.0.tgz", "@svgr/webpack": "4.3.3", "@testing-library/jest-dom": "^4.2.4", "@testing-library/react": "^9.4.0", diff --git a/packages/react/src/routing/LocationHistory.ts b/packages/react/src/routing/LocationHistory.ts index 2cff30bd3c..5b27767ade 100644 --- a/packages/react/src/routing/LocationHistory.ts +++ b/packages/react/src/routing/LocationHistory.ts @@ -55,7 +55,7 @@ export class LocationHistory { const routeInfos = this._getRouteInfosByKey(routeInfo.tab); if (routeInfos) { // If the latest routeInfo is the same (going back and forth between tabs), replace it - if (routeInfos[routeInfos.length - 1]?.id === routeInfo.id) { + if (this._areRoutesEqual(routeInfos[routeInfos.length - 1], routeInfo)) { routeInfos.pop(); } routeInfos.push(routeInfo); @@ -63,27 +63,27 @@ export class LocationHistory { this.locationHistory.push(routeInfo); } + private _areRoutesEqual(route1?: RouteInfo, route2?: RouteInfo) { + if(!route1 || !route2) { + return false; + } + return route1.pathname === route2.pathname && route1.search === route2.search; + } + private _pop(routeInfo: RouteInfo) { const routeInfos = this._getRouteInfosByKey(routeInfo.tab); - let ri: RouteInfo; + if (routeInfos) { - // Pop all routes until we are back - ri = routeInfos[routeInfos.length - 1]; - while (ri && ri.id !== routeInfo.id) { - routeInfos.pop(); - ri = routeInfos[routeInfos.length - 1]; - } - // Replace with updated route + // Pop the previous route + routeInfos.pop(); + // Replace the current route with an updated version routeInfos.pop(); routeInfos.push(routeInfo); } - ri = this.locationHistory[this.locationHistory.length - 1]; - while (ri && ri.id !== routeInfo.id) { - this.locationHistory.pop(); - ri = this.locationHistory[this.locationHistory.length - 1]; - } - // Replace with updated route + // Pop the previous route + this.locationHistory.pop(); + // Replace the current route with an updated version this.locationHistory.pop(); this.locationHistory.push(routeInfo); } diff --git a/packages/react/src/routing/__tests__/LocationHistory.spec.ts b/packages/react/src/routing/__tests__/LocationHistory.spec.ts new file mode 100644 index 0000000000..ff9fc612d9 --- /dev/null +++ b/packages/react/src/routing/__tests__/LocationHistory.spec.ts @@ -0,0 +1,219 @@ +import { LocationHistory } from '../LocationHistory'; +import { RouteInfo } from '../../models/RouteInfo'; +import { generateId } from '../../utils/generateId'; + +let log = false; + +describe('LocationHistory', () => { + let locationHistory: LocationHistory; + logHistory(); + log = true; + + beforeEach(() => { + locationHistory = new LocationHistory(); + }); + + describe('Popping history', () => { + describe('page1 > page2', () => { + beforeEach(() => { + const firstRoute = createRoute('/page1'); + const secondRoute = createRoute('/page2', firstRoute); + locationHistory.add(firstRoute); + locationHistory.add(secondRoute); + }); + + it('then should be at /page2 canGoBack should be true', () => { + expect(currentRoute().pathname).toEqual('/page2'); + expect(locationHistory.canGoBack()).toBeTruthy(); + }); + + it('when going back, should be on /page1 and canGoBack should be false', () => { + popRoute(); + expect(currentRoute().pathname).toEqual('/page1'); + expect(locationHistory.canGoBack()).toBeFalsy(); + }); + }); + + describe('tab1 > tab2 > Back', () => { + beforeEach(() => { + const tab1Route = createRoute('/tab1', undefined, 'tab1'); + const tab2Route = createRoute('/tab2', tab1Route, 'tab2'); + locationHistory.add(tab1Route); + locationHistory.add(tab2Route); + popRoute(); + }); + + it('then should be on tab1 and should not be able to go back', () => { + expect(currentRoute().pathname).toEqual('/tab1'); + expect(locationHistory.canGoBack()).toBeFalsy(); + }); + }); + + describe('tab1 > tab2 > tab3', () => { + beforeEach(() => { + const tab1Route = createRoute('/tab1', undefined, 'tab1'); + const tab2Route = createRoute('/tab2', tab1Route, 'tab2'); + const tab3Route = createRoute('/tab3', tab2Route, 'tab3'); + locationHistory.add(tab1Route); + locationHistory.add(tab2Route); + locationHistory.add(tab3Route); + }); + + it('when back once, then should be on tab2 and should be able to go back', () => { + popRoute(); + expect(currentRoute().pathname).toEqual('/tab2'); + expect(locationHistory.canGoBack).toBeTruthy(); + }); + + it('when going back twice, should be on tab1 and not be able to go back', () => { + popRoute(); + popRoute(); + expect(currentRoute().pathname).toEqual('/tab1'); + expect(locationHistory.canGoBack()).toBeFalsy(); + }); + }); + + describe('tab1 > tab1/details/1, then tab1/details/2', () => { + beforeEach(() => { + const homeRoute = createRoute('/tab1', undefined, 'tab1'); + const details1Route = createRoute('/tab1/details/1', undefined, 'tab1'); + const details2Route = createRoute('/tab1/details/2', undefined, 'tab1'); + locationHistory.add(homeRoute); + locationHistory.add(details1Route); + locationHistory.add(details2Route); + }); + + it('then should be at details2 and able to go back', () => { + expect(currentRoute().pathname).toEqual('/tab1/details/2'); + expect(locationHistory.canGoBack()).toBeTruthy(); + }); + + it('when going back, should be at details1 and able to go back', () => { + popRoute(); + expect(currentRoute().pathname).toEqual('/tab1/details/1'); + expect(locationHistory.canGoBack()).toBeTruthy(); + }); + + it('when going back twice, should be at home and not able to go back', () => { + popRoute(); + popRoute(); + expect(currentRoute().pathname).toEqual('/tab1'); + expect(locationHistory.canGoBack()).toBeFalsy(); + }); + }); + }); + + describe('Switching tabs', () => { + describe('tab1 > tab2 > tab1', () => { + beforeEach(() => { + const tab1Route = createRoute('/tab1', undefined, 'tab1'); + const tab2Route = createRoute('/tab2', tab1Route, 'tab2'); + const tab1_2Route = createRoute('/tab1', tab2Route, 'tab1'); + locationHistory.add(tab1Route); + locationHistory.add(tab2Route); + locationHistory.add(tab1_2Route); + }); + + it('then locationHistory should have 3 entries', () => + expect((locationHistory as any).locationHistory.length).toEqual(3)); + + it('then tab1 and tab2 should have one entry', () => { + expect((locationHistory as any).tabHistory['tab1'].length).toEqual(1); + expect((locationHistory as any).tabHistory['tab2'].length).toEqual(1); + }); + }); + + describe('tab1 > tab2 > tab3 > Back', () => { + beforeEach(() => { + const tab1Route = createRoute('/tab1', undefined, 'tab1'); + const tab2Route = createRoute('/tab2', tab1Route, 'tab2'); + const tab3Route = createRoute('/tab3', tab2Route, 'tab3'); + locationHistory.add(tab1Route); + locationHistory.add(tab2Route); + locationHistory.add(tab3Route); + popRoute(); + }); + + it('when going back, then locationHistory should have 2 entries', () => { + expect((locationHistory as any).locationHistory.length).toEqual(2); + }); + + it('when going back, then tab1, tab2, and tab3 should have one entry', () => { + expect((locationHistory as any).tabHistory['tab1'].length).toEqual(1); + expect((locationHistory as any).tabHistory['tab2'].length).toEqual(1); + expect((locationHistory as any).tabHistory['tab3'].length).toEqual(1); + }); + }); + }); + + describe('Replacing history', () => { + describe('tab1 > tab1/details/1, tab1/details/2, Replace to tab1/details/3', () => { + beforeEach(() => { + const homeRoute = createRoute('/tab1', undefined, 'tab1'); + const details1Route = createRoute('/tab1/details/1', homeRoute, 'tab1'); + const details2Route = createRoute('/tab1/details/2', details1Route, 'tab1'); + const details3Route = createRoute('/tab1/details/3', details2Route, 'tab1', 'replace'); + locationHistory.add(homeRoute); + locationHistory.add(details1Route); + locationHistory.add(details2Route); + locationHistory.add(details3Route); + }); + + it('then should be at details3', () => { + expect(currentRoute().pathname).toEqual('/tab1/details/3'); + }); + + it('tab1 history should have 3 entries and they should have correct paths', () => { + expect((locationHistory as any).tabHistory['tab1'].length).toEqual(3); + expect((locationHistory as any).tabHistory['tab1'][0].pathname).toEqual('/tab1'); + expect((locationHistory as any).tabHistory['tab1'][1].pathname).toEqual('/tab1/details/1'); + expect((locationHistory as any).tabHistory['tab1'][2].pathname).toEqual('/tab1/details/3'); + }); + + it('locationHistory should have 3 entries and they should have correct paths', () => { + expect((locationHistory as any).locationHistory.length).toEqual(3); + expect((locationHistory as any).locationHistory[0].pathname).toEqual('/tab1'); + expect((locationHistory as any).locationHistory[1].pathname).toEqual('/tab1/details/1'); + expect((locationHistory as any).locationHistory[2].pathname).toEqual('/tab1/details/3'); + }); + }); + }); + + function currentRoute() { + return locationHistory.current(); + } + + function popRoute() { + const previous = locationHistory.previous(); + previous.routeDirection = 'back'; + previous.routeAction = 'pop'; + locationHistory.add(previous); + } + + function logHistory() { + if (log) { + console.log('locationHistory', (locationHistory as any).locationHistory); + console.log('tabHistory', (locationHistory as any).tabHistory); + } + } +}); + +function createRoute( + pathname: string = '', + prevRoute?: RouteInfo, + tab?: string, + routeAction = 'push' +) { + const routeInfo: RouteInfo = { + id: generateId(), + lastPathname: prevRoute?.pathname, + prevRouteLastPathname: prevRoute?.lastPathname, + routeAction: routeAction as any, + routeDirection: 'forward', + pushedByRoute: prevRoute?.pathname, + pathname: pathname, + tab: tab, + search: '', + }; + return routeInfo; +} diff --git a/packages/react/tsconfig.json b/packages/react/tsconfig.json index 0224696dc0..4582ca37b6 100644 --- a/packages/react/tsconfig.json +++ b/packages/react/tsconfig.json @@ -24,7 +24,6 @@ "target": "es2017" }, "include": ["src/**/*.ts", "src/**/*.tsx"], - "exclude": ["**/__tests__/**"], "compileOnSave": false, "buildOnSave": false }