fix(vue-router): Prev route last pathname incorrect #24433
Conversation
…findLeavingViewItemByRouteInfo, small readability/debugging improvements
…in a correct RouteInfo state, added code readability
|
I try to use your PR. It works well. It's better then #24409 . I migrate your PR to the v6 version in this branch. |
sean-perkins
changed the title
There was a problem hiding this comment.
We will want to clean-up the commented out code & format any inline code documentation to match existing format, but that can come at a later date.
If there's an opportunity to write tests to cover functional changes to prevent the behavior regressing in the future, I would encourage it.
Other Core members will provide feedback and insight as they are able. Thanks for putting this PR together 👍
packages/vue-router/src/router.ts
Outdated
| nextRouteInfo.lastPathname = leavingRouteInfo.pathname; | ||
| nextRouteInfo.tab = leavingRouteInfo.tab; | ||
| //@TODO: Check if bug, why would pathname be blank? | ||
| nextRouteInfo.pushedByRoute = (leavingRouteInfo.pathname !== '') ? leavingRouteInfo.pathname : undefined; |
There was a problem hiding this comment.
I believe you can create an instance where pathname is '', when using history.pushState/history.replaceState, but also I cannot immediately think of an instance where that's appropriate. From the docs, it appears it happens when there is no path (https://developer.mozilla.org/en-US/docs/Web/API/Location/pathname).
There was a problem hiding this comment.
I'll add this comment to explain the blank scenario in code
There was a problem hiding this comment.
I saw the first route has path = '' before we redirect it to the desired page too. Also I did break tabs so I will fix that and remove commented out code and add test cases for my bug scenario and the tab scenario. Pretty sure there are still bugs when navigating around with push,replace,go,back in complex scenarios so I will see what I find.
@sean-perkins When going between tabs on the same ion-tab-bar do we want each new tab to be pushed onto locationHistory or should the tab replace the previous tab if they keep clicking between tabs? The code base seems to handle tabs in unique ways and I was wondering if we could just have them behave as replacements, that should simplify things.
… to tabs so back button resolves to the last non-tab page
|
Last commit fixes spamming tabs and clicking the ion back button taking you to the last non tab page, but using android device back button only goes -1 to the last tab, or whatever is the last route |
packages/vue-router/src/router.ts
Outdated
| @@ -265,8 +265,11 @@ export const createIonRouter = (opts: IonicVueRouterOptions, router: Router) => | |||
| nextRouteInfo.pushedByRoute = leavingRouteInfo?.pathname; | |||
| nextRouteInfo.lastPathname = leavingRouteInfo?.pathname; | |||
| } else if (nextRouteInfo.routerAction === 'push' && isNewTab) { | |||
| // const lastTabRouteInfo = locationHistory.getCurrentRouteInfoForTab(nextRouteInfo.tab); | |||
| nextRouteInfo.pushedByRoute = (leavingRouteInfo.pathname !== '') ? leavingRouteInfo.pathname : undefined; | |||
| if (isLeavingRouteTab) { | |||
There was a problem hiding this comment.
My thoughts behind this were if any last route was a tab, use its pushedByRoute(which would always be a non-tab route) else use the lastPathname. So if the user pushes ion-back-button it would go to a non-tab page, but android device back would just go back -1
|
Route.replace is working great, tab pages ion-back-button and device back button working great, navigating back to tabs from non-tab page sets the proper children to render, last issue is undefined viewItem when using router.go from another ticket: #24432 |
…rning for undefined viewItem so you can still debug the condition/detect when it occurs.
|
Hi, I found some bugs in Another problem: |
|
When pushing -> replacing -> pushing -> pop backing -> replacing -> pop backing, the route will wrong. |
I manually added your changes here: d6c7ed2 Did I not push the change in? |
|
Digging into the cypress tests I noticed that clicking tabs creates an ion-page in another ion-router-outlet:
Which the tests will probably find the wrong 'tab2secondary'? @sean-perkins does this seem odd? |
|
The only change from old behavior is clicking the back button does not remove/unmount the viewItem. The only time that occurs is when 'replace' is fired. The reason for this is if you have a 'forward' and 'back' button like a browser you would expect to be able to go forward or back. |
…utlet. [data-pageid="tab1-secondary"]:nth-of-type(2) is now [data-pageid="tab1-secondary"]:nth-of-type(1) as it only exists on the correct ion-router-outlet
packages/vue-router/src/router.ts
Outdated
| @@ -62,6 +66,13 @@ export const createIonRouter = (opts: IonicVueRouterOptions, router: Router) => | |||
| const initialHistoryPosition = opts.history.state.position as number; | |||
| let currentHistoryPosition = opts.history.state.position as number; | |||
|
|
|||
| const currentHistoryIndex = () => { | |||
| let index = (currentHistoryPosition) - initialHistoryPosition; | |||
| console.log('Current index is: ', index); | |||
There was a problem hiding this comment.
Let's clean-out any temporary testing logs, comments, commented out code, etc.
packages/vue-router/src/router.ts
Outdated
| @@ -62,6 +66,13 @@ export const createIonRouter = (opts: IonicVueRouterOptions, router: Router) => | |||
| const initialHistoryPosition = opts.history.state.position as number; | |||
| let currentHistoryPosition = opts.history.state.position as number; | |||
|
|
|||
| const currentHistoryIndex = () => { | |||
There was a problem hiding this comment.
We should be able to inline this:
const currentHistoryIndex = () => currentHistoryPosition - initialHistoryPosition;
There was a problem hiding this comment.
The only thing I would say about this is debugging isn't possible.
Short hand is fun and all for rapidly writing code but if you ever need to go back and debug, then you have to refactor it.
packages/vue-router/src/router.ts
Outdated
| /** | ||
| * If there is no route id, assign one, else use incoming route id for location history lookup | ||
| */ | ||
| if (incomingRouteParams.hasOwnProperty('id') === false) { |
There was a problem hiding this comment.
We tend to do undefined checks and object-based notation,.
if (typeof incomingRouteParams.id === 'undefined') {
incomingRouteParams.id = generateId('routeInfo');
}
|
Hello @yoyo930021 and @tigohenryschultz, I have proposed an alternative fix for this issue and attached a dev build for testing in #24432 (comment). I would really appreciate it if you could test the dev build in your apps and let me know if it resolves the issue that was reported in #24432. If it does not resolve the issue, please send over a sample application I can use to reproduce the issue, and I will be more than happy to take another look to see if we can better address the issue. Thanks! |
|
Hi there, Thank you for your PR! We appreciate all the hard work you and @yoyo930021 put into creating your respective solutions as well as your eagerness to improve Ionic. I want to provide some updates regarding this PR as well as #24432, the issue this PR was created to address. I have proposed an alternative solution in #24721 which fixes the issue in #24432. The fix has been merged into the As for this PR, we are unable to accept this pull request. There are a couple reasons for this:
The changes here appear to make Ionic routing behave more like how routing works in a web browser than in a native mobile app. Ionic routing is designed to behave like the latter, so the changes proposed here are not a good fit for Ionic. We are looking to provide an experience that behaves like a native mobile application, not a mobile web app. I understand you may still have questions or changes you wish to see made in Ionic. I encourage you to open issues where each issue is dedicated to a particular concern you have in Ionic. From there, we can discuss and answer any questions you may have. I am going to close this PR, but please feel free to open up separate issues for each concern you have. Thanks! |
|
Thanks for working hard on these issues @liamdebeasi However, we'll still have to use our custom build until all other routing issues are resolved but appreciate the effort. One of the fixes in my pull request that is still an issue in master is that all tabs now have a proper life cycle. To accomplish this every ion-router-outlet will need to be able to handle route changes, transition animations also had to refactored to accommodate that. I know previously you didn't want to solve that problem and suggested watching the route object, is this the same position you hold? If not I can create a separate ticket/test case and detail the issue. Also I deprecated prevRoute_lastPathname as it was a hack used to remember which tab to hide when navigating back into a nested ion-router-outlet tab. Could I submit a pull request with just additional test cases for the above scenarios? |
|
Hey @tigohenryschultz, Sorry for the delay! I recently got a new computer but did not realize my email was not properly configured, so I never saw this comment (which explains why my inbox was empty all the time 😄 ) Just so I understand, you want the individual tab items to fire lifecycle events even when navigating to and from tabs? If so, I think that makes sense to do. Could you create a separate ticket as you mentioned so we can better understand the issue? Thanks! |
|
Also just as a heads up: We are planning to consolidate a lot of our routing code across Angular, React, and Vue to make the routing integration easier to maintain. Part of that consolidation may result in some of these improvements you are hoping to see too. |
Pull request checklist
Please check if your PR fulfills the following requirements:
npm run build) was run locally and any changes were pushednpm run lint) has passed locally and any fixes were made for failuresPull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: #24432
What is the new behavior?
Does this introduce a breaking change?
Other information
I will be testing the router.go logic to see if anything not tested broke because I did change how replace worked in locationHistory, code review would be great for now.