Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: router.replace eventually breaks routing #24432

Closed
4 of 6 tasks
tigohenryschultz opened this issue Dec 17, 2021 · 23 comments · Fixed by #24721
Closed
4 of 6 tasks

bug: router.replace eventually breaks routing #24432

tigohenryschultz opened this issue Dec 17, 2021 · 23 comments · Fixed by #24721
Labels
package: vue @ionic/vue package type: bug a confirmed bug report

Comments

@tigohenryschultz
Copy link
Contributor

tigohenryschultz commented Dec 17, 2021

Prerequisites

Ionic Framework Version

  • v4.x
  • v5.x
  • v6.x

Current Behavior

After navigating around using router.push and router.replace, eventually a view will linger.

Expected Behavior

Correct rendering of leaving/entering view items.

Steps to Reproduce

Navigating from View A1 -(replace)-> B/1 -(push)-> B/2 -(back button)-> B/1 -(replace)-> A1 -(replace)-> B1

B1 never gets render and is glitched onto A1.

Code Reproduction URL

No response

Ionic Info

Ionic:

Ionic CLI : 6.16.3

Utility:

cordova-res : not installed globally
native-run : not installed globally

System:

NodeJS : v14.17.3
npm : 6.14.13
OS : Windows 10

Additional Information

No response

@ionitron-bot ionitron-bot bot added the holiday triage issues that were created during holiday period label Dec 17, 2021
@ionitron-bot
Copy link

ionitron-bot bot commented Dec 17, 2021

Thanks for the issue! This issue has been labeled as holiday triage. With the winter holidays quickly approaching, much of the Ionic Team will soon be taking time off. During this time, issue triaging and PR review will be delayed until the team begins to return. After this period, we will work to ensure that all new issues are properly triaged and that new PRs are reviewed.

In the meantime, please read our Winter Holiday Triage Guide for information on how to ensure that your issue is triaged correctly.

Thank you!

@sean-perkins sean-perkins added the package: vue @ionic/vue package label Dec 17, 2021
@ionitron-bot ionitron-bot bot removed the holiday triage issues that were created during holiday period label Dec 17, 2021
@sean-perkins
Copy link
Contributor

Partially related to: #24226

We may decide to group the Vue routing issues together to reduce individual & overlapping tasks for us internally.

@sean-perkins sean-perkins added the type: bug a confirmed bug report label Dec 17, 2021
@tigohenryschultz
Copy link
Contributor Author

Partially related to: #24226

We may decide to group the Vue routing issues together to reduce individual & overlapping tasks for us internally.

Since I have a good understand of routing now I don't mind tackling the issues/tickets regarding it, they're pretty high priority issues for the company I work for and we don't want any negative app reviews from routes breaking so I've been given the go ahead to work on this.

@tigohenryschultz
Copy link
Contributor Author

Working on this issue in this pull request: #24433

@liamdebeasi
Copy link
Contributor

@tigohenryschultz Do you have a reproduction case for this router.replace issue? I have a fix for #24226 and would like to test to see if my patch also fixes this issue.

Note that this patch does not fix #23873 since that example does not use replace.

@tigohenryschultz
Copy link
Contributor Author

@tigohenryschultz Do you have a reproduction case for this router.replace issue? I have a fix for #24226 and would like to test to see if my patch also fixes this issue.

Note that this patch does not fix #23873 since that example does not use replace.

I've been side tracked getting the original test cases to pass but I do plan on adding tests for this specific scenario I ran into and the other ones I found while refactoring the code. Like:

Sometimes a ViewItem would be added to multiple outlets
The last tab you were on would not have ion-page-hidden when navigating away until re-entering the tab so the life cycles would not fire for it.

On Monday I can add a test case for the specific scenario I ran into

@Lokilein
Copy link

I think I got a similar/ the same issue (details here => Ionic Forum: Ionic 5/6 with Vue: Pages do not change sometimes (URL does) )
I tried some things, but using push instead of replace or back seems to help. I updated to ionic/vue 6.0.2 and still got the issue.

@tigohenryschultz
Copy link
Contributor Author

I've fixed this issue in a pull request but they have not merged it in yet.

@liamdebeasi
Copy link
Contributor

Hi @Lokilein and @tigohenryschultz,

I am investigating a fix for this issue. However, in order to verify the fix I need to test against a sample reproduction of the issue reported in this thread.

Can you provide a GitHub repo that I can use to reproduce the issue described in #24432 (comment)?

@tigohenryschultz
Copy link
Contributor Author

prevRouteLastPathname becomes incorrect after some navigation steps causing the IonRouterOutlet to not find the correct 'leaving' viewItem. I'll try to create a sample if I have time but it is fixed in the pull request.

@liamdebeasi
Copy link
Contributor

Thanks, a GitHub repo would be great. Even if the resolution here is to move forward with your PR, we would still need the sample app.

@tigohenryschultz
Copy link
Contributor Author

Not sure if thats possible, there's a lot of changes to the code and behavior to tabs to just simplify everything and reduce bugs/behave like a native browser going forward

@liamdebeasi
Copy link
Contributor

Let's start with the GitHub repo for this particular bug and then we can assess from there. Thanks!

@Lokilein
Copy link

Lokilein commented Feb 4, 2022

Hi there. I cannot share my repository, but I've recreated the bug here with instructions to reproduce it:
https://github.com/Lokilein/ionicRoutingBug

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Feb 4, 2022

Thanks! This particular reproduction does not use router.replace which is what is causing the issue reported in this thread. Does your repo reproduce a different issue?

@Lokilein
Copy link

Lokilein commented Feb 4, 2022

Hmm, actually I am not completly sure. The original description also used the back action and the result is the same. I have to check if replace makes any difference, but I think it causes the same issue, that's why I replaced everything with push in my project for now.

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Feb 4, 2022

Ok thanks. I have a potential solution that I am working on now for your scenario. I will post a dev build here when it is ready for testing.

@Lokilein
Copy link

Lokilein commented Feb 4, 2022

Okay I've updated my example and now it fails faster by using the back button the first time. URL is changing but the page remains. I'm using replace now.

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Feb 4, 2022

Thanks! Can you try the following dev build and let me know if it resolves the issue?

npm install @ionic/vue@6.0.6-dev.1643998066.2660c83 @ionic/vue-router@6.0.6-dev.1643998066.2660c83

This dev build fixes the main replace issue in this thread as well as the other issue you reported in #24432. (Turns out they were related bugs 😄 )

@Lokilein
Copy link

Lokilein commented Feb 7, 2022

This looks much better! I cannot reproduce any issue with this anymore, neither with back nor with replace 👍 Thank you very much!

Can you tell when this will be in main?

@liamdebeasi
Copy link
Contributor

I do not have an estimate of when this fix will be merged as it still needs to be reviewed by the team.

liamdebeasi added a commit that referenced this issue Feb 7, 2022
resolves #24432

Co-authored-by: tigohenryschultz <tigohenryschultz@users.noreply.github.com>
Co-authored-by: yoyo930021 <yoyo930021@users.noreply.github.com>
@liamdebeasi
Copy link
Contributor

Thanks for the issue. This has been resolved via #24721 and a fix will be available in an upcoming release of Ionic Framework.

Please feel free to continue testing the dev build.

@ionitron-bot
Copy link

ionitron-bot bot commented Mar 9, 2022

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Mar 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: vue @ionic/vue package type: bug a confirmed bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants