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.go with tabs causes undefined references #24303

Closed
4 of 6 tasks
tigohenryschultz opened this issue Dec 1, 2021 · 33 comments · Fixed by #25206
Closed
4 of 6 tasks

bug: router.go with tabs causes undefined references #24303

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

Comments

@tigohenryschultz
Copy link
Contributor

tigohenryschultz commented Dec 1, 2021

Prerequisites

Ionic Framework Version

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

Current Behavior

When navigating around the user selects a go back button that takes our user back to a specific page. That page could be -3 pages, -7 or any number of pages. This works fine for a few tries but eventually we receive the Ionic bug. I have the ionBackButton binded to trigger my own go back logic that will trigger route.go(n).

Expected Behavior

Router.go(n) should work regardless of a large, small, or negative number.

Steps to Reproduce

Have a few components and route between them, navigate to a page with tabs and click between the tabs, trigger route.go(-3) to go back a few times using an override like:

document.addEventListener('ionBackButton', this.deviceBackClicked, true);

 deviceBackClicked: function (event) {
        // event.stopPropagation();
        event.stopImmediatePropagation();
        this.$router.go(-3)
}

Code Reproduction URL

No response

Ionic Info

Ionic:

Ionic CLI : 6.18.1 (C:\Users\KingSmurf\AppData\Roaming\nvm\v12.22.3\node_modules@ionic\cli)
Ionic Framework : @ionic/vue 5.9.1

Capacitor:

Capacitor CLI : 2.5.0
@capacitor/android : 2.5.0
@capacitor/core : 2.5.0
@capacitor/ios : 2.5.0

Utility:

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

System:

NodeJS : v12.22.3 (C:\Program Files\nodejs\node.exe)
npm : 6.14.13
OS : Windows 10

Additional Information

I have a feeling it is related to ionic tabs and have not encountered this problem if I test using pages without tabs.

image

You can see it is accessing undefined information, crashing the app

Uncaught (in promise) TypeError: Cannot set property 'mount' of undefined
at Object.unmountLeavingViews (index.esm.js:690)
at handlePageTransition (index.esm.js:1095)

@ionitron-bot ionitron-bot bot added the triage label Dec 1, 2021
@amandaejohnston amandaejohnston self-assigned this Dec 2, 2021
@amandaejohnston
Copy link
Contributor

Thanks for the report. Could you please create an Ionic starter app that reproduces the issue and post a link to the repo?

Aside: this may be similar to #22563.

@amandaejohnston amandaejohnston added the ionitron: needs reproduction a code reproduction is needed from the issue author label Dec 2, 2021
@ionitron-bot
Copy link

ionitron-bot bot commented Dec 2, 2021

Thanks for the issue! This issue has been labeled as needs reproduction. This label is added to issues that need a code reproduction.

Please reproduce this issue in an Ionic starter application and provide a way for us to access it (GitHub repo, StackBlitz, etc). Without a reliable code reproduction, it is unlikely we will be able to resolve the issue, leading to it being closed.

If you have already provided a code snippet and are seeing this message, it is likely that the code snippet was not enough for our team to reproduce the issue.

For a guide on how to create a good reproduction, see our Contributing Guide.

@ionitron-bot ionitron-bot bot removed the triage label Dec 2, 2021
@amandaejohnston amandaejohnston removed their assignment Dec 2, 2021
@tigohenryschultz
Copy link
Contributor Author

tigohenryschultz commented Dec 2, 2021

Reproduced here: https://github.com/tigohenryschultz/router-undefined-reference

To reproduce use the navigation buttons to navigate to the tabs pages, click between the tabs and use the 'GO BACK PAGE 1' or 'GO BACK PAGE 3' button. Sometimes you have to do this a few times, but shouldn't take more than a few minutes to see the attached undefined reference bug in the router code.

We are keeping track of the navigation stack to see how many times we need to go back, so the router.go delta can be -3, -5, -10.

Eventually youll see this bug:

image

@amandaejohnston amandaejohnston added triage and removed ionitron: needs reproduction a code reproduction is needed from the issue author labels Dec 2, 2021
@amandaejohnston amandaejohnston self-assigned this Dec 3, 2021
@amandaejohnston
Copy link
Contributor

I'm unable to replicate the error using that repo and following your instructions. Would you be able to give more specific steps to replicate? A recording of your own might also help.

@amandaejohnston amandaejohnston added the needs: reply the issue needs a response from the user label Dec 3, 2021
@ionitron-bot ionitron-bot bot removed the triage label Dec 3, 2021
@amandaejohnston amandaejohnston removed their assignment Dec 3, 2021
@tigohenryschultz
Copy link
Contributor Author

Sure @amandaesmith3 , give me a few minutes

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Dec 3, 2021
@tigohenryschultz
Copy link
Contributor Author

The main way to reproduce it is to keep navigating back to the tabs, click a few tabs, then click 'GO BACK PAGE 1' or 'GO BACK PAGE 3'

video-1638564768.mp4

These are the consoles errors I received:

image

Once you see the log:

index.esm.js?bec5:690 Uncaught (in promise) TypeError: Cannot set properties of undefined (setting 'mount')

Usually the interface will then lock up, as seen in this video:

video-1638564905.mp4

@amandaejohnston
Copy link
Contributor

Thank you, I'm able to reproduce now 👍 After some digging and internal discussion, the underlying cause here seems to be the same as #24332. So, I'm going to close this issue in favor of tracking progress there. I'll post your reproduction and the steps I found to consistently trigger the errors on that thread.

If the cause turns out to be different, we can re-open this issue and go from there. Thanks for using Ionic!

@liamdebeasi
Copy link
Contributor

Reopening as per #24332 (comment)

@liamdebeasi liamdebeasi reopened this Dec 9, 2021
@amandaejohnston
Copy link
Contributor

The consistent reproduction steps I found:

  1. Go to Page 2
  2. Go to Page 3
  3. Go to Tab 1
  4. Tab 2
  5. Tab 1
  6. Go Back Page 3 (error)
  7. Go Back Page 3 again
  8. Go to Tab 1
  9. Go Back Page 1 (different error)

@amandaejohnston amandaejohnston added package: vue @ionic/vue package type: bug a confirmed bug report labels Dec 10, 2021
@ionitron-bot ionitron-bot bot removed the triage label Dec 10, 2021
@amandaejohnston amandaejohnston changed the title bug: router.go with large numbers causes undefined references bug: router.go with tabs causes undefined references Dec 10, 2021
@tigohenryschultz
Copy link
Contributor Author

@amandaesmith3 When you want to debug these sample projects, how do you install the ionic-framework codebase locally for debugging/fixing? I would happily look into this bug and submit a pull request but I was unable to install the ionic-framework locally using

npm i C:\repos\ionic-framework\packages\vue-router\

Kept getting one eslint/webpack problem after another. Is there a guide on installing locally for debugging?

@amandaejohnston
Copy link
Contributor

@tigohenryschultz
Copy link
Contributor Author

We have a contributing guide here: https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#creating-a-pull-request

Those instructions are for testing changes in the core, not in my code base. I was talking about installing the vue-router locally to https://github.com/tigohenryschultz/router-undefined-reference

How else would you know if you fixed the bug if you didn't install the vue-router codebase locally to a project? Essential this project: https://github.com/tigohenryschultz/router-undefined-reference would reference the local copy of vue-router instead of the npmjs published one, so I can test code changes in real-time.

Is this not how you guys fix bugs from projects submitted in tickets?

@liamdebeasi
Copy link
Contributor

We typically use npm pack to build the changes and copy to a test project or we use https://github.com/ionic-team/ionic-framework/blob/main/package.json#L6 to create an installable dev build that others can install.

The latter is only available to Ionic Team members since npm access is required, so the easiest is probably to do the following:

  1. Make your changes in the Vue Router directory.
  2. Build the changes.
  3. Run npm pack to pack the built changes.
  4. Install those packed changes in your test app.

Here are the instructions for building Vue and Vue Router:

Vue: https://github.com/ionic-team/ionic-framework/tree/main/packages/vue
Vue Router: https://github.com/ionic-team/ionic-framework/tree/main/packages/vue-router

For myself, I usually go into the compiled output inside of node_modules in my example repo and make changes there to debug quickly. The Vue CLI should detect the changes and refresh the app. I find that to be easier/faster than doing npm pack or a dev build.

@tigohenryschultz
Copy link
Contributor Author

We typically use npm pack to build the changes and copy to a test project or we use https://github.com/ionic-team/ionic-framework/blob/main/package.json#L6 to create an installable dev build that others can install.

The latter is only available to Ionic Team members since npm access is required, so the easiest is probably to do the following:

  1. Make your changes in the Vue Router directory.
  2. Build the changes.
  3. Run npm pack to pack the built changes.
  4. Install those packed changes in your test app.

Here are the instructions for building Vue and Vue Router:

Vue: https://github.com/ionic-team/ionic-framework/tree/main/packages/vue Vue Router: https://github.com/ionic-team/ionic-framework/tree/main/packages/vue-router

For myself, I usually go into the compiled output inside of node_modules in my example repo and make changes there to debug quickly. The Vue CLI should detect the changes and refresh the app. I find that to be easier/faster than doing npm pack or a dev build.

Will give that a go.

For other npm packages we normally we are able to install development npm packages locally like:

npm i c:\repos\ionic-framework\packages\vue-router

But eslint/webpack is not happy with something in the vue-router packages. This is how we are normally able to work on our npmjs packages, we can change them in the project directly and since npm is referencing the folder as a symbolic link it can watch/rebuild it when necessary instead of writing code in the compiled output. I tried tweaking stuff in your vue-router package but wasn't sure what the issue was, I think its something in the package.json causing issues with eslint/webpack. Would be nice to figure that problem out one day, will make debugging a lot smoother.

But I'll give your steps a go and see what I can dig up, thanks!

@liamdebeasi
Copy link
Contributor

Interesting, what ESLint/Webpack errors were you getting?

@tigohenryschultz
Copy link
Contributor Author

Interesting, what ESLint/Webpack errors were you getting?

I created this forum post here:

https://forum.ionicframework.com/t/installing-ionic-vue-router-locally-with-npm-install-path/218188/2

Seems like it was missing some eslintConfig properties to the package.json in @ionic/vue-router and then in our ionic app we had to make other changes but kept running into different errors.

Here is a picture of one of our npmjs packages package.json that we are able to install locally like:

npm i c:\repos\tigoapps\packages\tigobuild

image

This way I can write code/debug directly here and the watcher can rebuild in realtime from this folder.

@sean-perkins
Copy link
Contributor

Spent a little time researching the two errors being thrown, here's what I've found so far:

While following Amanda's replication steps, when performing step 6 (Go back to Page 3), the look-up for the entering view does not return a result.

findViewItemByRouteInfo(routeInfo, id); // undefined

where routeInfo is:

{
  delta: -1,
  id: "3",
  lastPathname: "/tabs/tab1",
  params: {},
  pathname: "/page3",
  position: 15,
  prevRouteLastPathname: "/page1",
  pushedByRoute: "/page2",
  routerAction: "pop",
  routerDirection: "back",
  search: ""
}

and id is 2.

The second time that it is successful, the routeInfo is mostly the same, with the addition of: tab: undefined. The id is now 1. This will return an entering view item and transition successfully.

The second exception that occurs with step 9 (Going back to Page 1), is a result of the delta being -4, but the delta is larger than the view stack size, which results in trying to unmount a view that doesn't exist. I cannot tell at this time, if this is a side-effect of the navigation failing with the first exception.

@liamdebeasi
Copy link
Contributor

If findViewItemByRouteInfo is undefined this is usually (but not always) an indication that the issue is somewhere in https://github.com/ionic-team/ionic-framework/blob/main/packages/vue-router/src/router.ts.

@Jannik-KJ
Copy link

I'm facing the exact same issue. Navigating back and forth and jumping between tabs causes Uncaught (in promise) TypeError: Cannot set properties of undefined (setting 'mount') and Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'classList')
Hope you are able to find a solution or a temporary fix

@liamdebeasi
Copy link
Contributor

Thanks for the issue. Can everyone try the following dev build, and let me know if it resolves the issue?

npm install @ionic/vue@6.1.3-dev.11651080471.167ddf56 @ionic/vue-router@6.1.3-dev.11651080471.167ddf56

A couple notes:

  1. There is a lingering issue with browser back button/hardware back button that is being tracked in bug: vue, react using browser back button in tabs does not display correct url #25141

  2. There appears to be a bug in the sample app provided in bug: router.go with tabs causes undefined references #24303 (comment).

After following the steps in #24303 (comment) up to step 5, clicking "Go back Page 3" performs router.go(-1). However, that should bring you back to Tab 2, not Page 3. With tabs, we push a new route entry every time you switch tabs.

So when you do Page 3 --> Tab 1 --> Tab 2 --> Tab 1, the history stack looks something like this:

stack (most recent route on top)
/tabs/tab1
/tabs/tab2
/tabs/tab1
/page3

This is aligned with mobile tabs behavior. To compare, open a tabbed application on an Android phone and do Tab 1 --> Tab 2 --> Tab 1. Pressing the hardware back button should bring you back to Tab 2.

For this particular app, it looks like it should be performing router.go(-3) instead.

@MimyyK
Copy link

MimyyK commented Apr 28, 2022

FYI, I tried to follow these steps in this related issue #24936 and it fixed my issue

@Jannik-KJ
Copy link

Thanks for the issue. Can everyone try the following dev build, and let me know if it resolves the issue?

npm install @ionic/vue@6.1.3-dev.11651080471.167ddf56 @ionic/vue-router@6.1.3-dev.11651080471.167ddf56

A couple notes:

  1. There is a lingering issue with browser back button/hardware back button that is being tracked in bug: vue, react using browser back button in tabs does not display correct url #25141
  2. There appears to be a bug in the sample app provided in bug: router.go with tabs causes undefined references #24303 (comment).

After following the steps in #24303 (comment) up to step 5, clicking "Go back Page 3" performs router.go(-1). However, that should bring you back to Tab 2, not Page 3. With tabs, we push a new route entry every time you switch tabs.

So when you do Page 3 --> Tab 1 --> Tab 2 --> Tab 1, the history stack looks something like this:

stack (most recent route on top)
/tabs/tab1
/tabs/tab2
/tabs/tab1
/page3
This is aligned with mobile tabs behavior. To compare, open a tabbed application on an Android phone and do Tab 1 --> Tab 2 --> Tab 1. Pressing the hardware back button should bring you back to Tab 2.

For this particular app, it looks like it should be performing router.go(-3) instead.

Hi, with this build i'm still getting Uncaught (in promise) TypeError: Cannot set properties of undefined (setting 'mount')
at Object.unmountLeavingViews (index.esm.js:900:1)
at handlePageTransition (index.esm.js:1320:1)

@liamdebeasi
Copy link
Contributor

@Jannik-KJ It's hard to say what the issue may be without a reproduction. Can you provide a GitHub repo I can use to reproduce the issue?

@liamdebeasi
Copy link
Contributor

Also, it may be better to start a separate thread. I recall mentioning this in #25017 (comment). It's possible that the issue you are running into is a different issue.

@liamdebeasi
Copy link
Contributor

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

Please feel free to continue testing and let me know if you run into any issues.

@Jannik-KJ
Copy link

Jannik-KJ commented May 1, 2022

@liamdebeasi I have created a clean project with a single child route and was able to reproduce it.
https://github.com/Jannik-KJ/ionic-6-router
Let me know if you have any questions or want me to create a bug instead :)

@tigohenryschultz
Copy link
Contributor Author

I noticed that the tab history stacks indefinitely now but in my limited testing I did not see any issue. If a user clicks between two tabs 100 times they will need to click back 100 times, to my understanding that is not designed behavior as the tab history stack is tracked for each unique outlet/tab.

@liamdebeasi
Copy link
Contributor

So the main thing here we need to be concerned about is the hardware back button on Android (aka going back multiple times as you noted). The browser back button also applies, but I am going to reference only the hardware back button for this example.

Unfortunately, Android apps do not have a consistent tabs interaction pattern. There are a few patterns we could adopt:

Gmail Pattern:

Pressing the hardware back button on the root of a tab jumps you back to the initial tab.

Example: Mail --> Chat --> Spaces --> Meet. Pressing the hardware back button once will take you from Meet to Mail. Notice that we never go back to Chat or Spaces.


Spotify Pattern:

Pressing the hardware back button on the root of a tab jumps you back to the previous tab you were on.

Example: Home --> Search --> Your Library.

First hardware back button press: You are taken back to Search.
Second hardware back button press: You are taken back to Home.

In this example, you can switch between Home and Search 100 times. You will need to press the hardware back button 100 times as well.

This is the pattern Ionic follows as it is easiest to implement/reason about. Note: There is a bug where pushing child views within tabs causes this to not work as intended. We are tracking this in #25213.


Google Play Pattern:

Pressing the hardware back button on the root of a tab jumps you back to the previous tab you were on. However, it will never bring you back to the same tab twice.

Example: Games --> Apps --> Movies & TV --> Apps.

First hardware back button press: You are taken back to Movies & TV.
Second hardware back button press: You are taken back to Games. Note that we have skipped over showing Apps again because we already visited it for a second time.


Google Drive Pattern:

Pressing the hardware back button on the root of a tab closes the app.

Example: Home --> Starred --> Shared --> Files

Pressing the hardware back button on the Files tab just closes the app.


Honestly what's most concerning here is there are three Google made apps that each have drastically different tabs behaviors. As a result, it's hard to say which behavior is "correct". As I mentioned, we currently implement the Spotify pattern, but this is something worth discussing with the team as we seek to clean up our routing integration.

@liamdebeasi
Copy link
Contributor

One idea we are investigating is a "hardware back button strategy API". This would let developers customize the behavior when going back. Ionic would provide some of the more common behaviors as standalone modules, and developers can choose which one they want to use.

@tigohenryschultz
Copy link
Contributor Author

I'm fine with whatever you guys decide. More control for the users is always great.

@liamdebeasi
Copy link
Contributor

Thanks for pointing this behavior by the way -- I did not know that our behavior was not the only tabs behavior until I dug into this a bit more.

@piotr-cz
Copy link

I'm receiving similar issue reports from Sentry, but I'm using React (@ionic/react + @ionic/react-router 6.1.6)

Apparently this happens when users switches tabs ~ 3 times within 2 seconds. I'm not able to tell if user was pressing hardware back button or using buttons to switch tabs.

I was not able to reproduce it either in browser nor in emulator.

@ionitron-bot
Copy link

ionitron-bot bot commented Jun 29, 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 Jun 29, 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.

7 participants