bug: ion-router can set wrong url after nav changes

[cjorasch]

Prerequisites

• I have read the Contributing Guidelines.
• I agree to follow the Code of Conduct.
• I have searched for existing issues that already report this problem, without success.

Ionic Framework Version

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

Current Behavior

If the same component is used to render more than one route then the router can set the wrong url after navigation.

For example, when the user clicks the <ion-back-button> the url can be left unchanged or can be cleared entirely.

Any code that looks at the url will be using an incorrect value, the browser history will be incorrect, refreshing the page will change the content, etc.

Expected Behavior

Navigating back should result in the router setting the correct url.

Steps to Reproduce

If the following routes are defined:

<ion-router useHash>
    <ion-route url="/:s1/:s2/:s3" component="my-page" />
    <ion-route url="/:s1/:s2" component="my-page" />
    <ion-route url="/:s1" component="my-page" />
    <ion-route url="" component="my-page" />
</ion-router>

And the user navigates through the following steps:

1. start at "/"
2. navigate to "/a"
3. navigate to "/b"
4. click on the back button
5. the page content for "/a" is displayed
6. the url shows "/a/b"

If the routes are defined in reverse order:

<ion-router useHash>
    <ion-route url="" component="my-page" />
    <ion-route url="/:s1" component="my-page" />
    <ion-route url="/:s1/:s2" component="my-page" />
    <ion-route url="/:s1/:s2/:s3" component="my-page" />
</ion-router>

And the user navigates through the following steps:

1. start at "/"
2. navigate to "/a"
3. navigate to "/b"
4. click on the back button
5. the page content for "/a" is displayed
6. the url shows "/"

After clicking on the ionic back button, <ion-nav> correctly transitions to the previous page and then calls router.navChanged() to update the url path.

This goes through this process:

// router.tsx - .navChanged()
const chain = routerIDsToChain(ids, routes);

// chain now matches the first route with the same component name
// it does not use any of the path values to determine the best match.
// In the above examples it will always match the first route that is defined

const path = chainToPath(chain);

// path.ts
export const chainToPath = (chain: RouteChain): string[] | null => {
  const path = [];
  for (const route of chain) {
    for (const segment of route.path) {
      if (segment[0] === ':') {
        const param = route.params && route.params[segment.slice(1)];
        if (!param) {
          return null;
        }
        path.push(param);
      } else if (segment !== '') {
        path.push(segment);
      }
    }
  }
  return path;
};

// This results in two potential error cases
// If the route for "/" is defined first then it will return []
// If the route for "/:a/:b/:c" is defined first it will return null

// router.tsx
this.setPath(path, direction);

// This will result in clearing the url path (if [] was returned)
// or leaving the url path unchanged (if null was returned)

Note: Clicking on the browser back button (instead of using <ion-nav> to pop the view) does not result in this problem because the path is correct and nav is able to show the page.

Code Reproduction URL

No response

Ionic Info

Ionic:

Ionic CLI : 6.12.1

Utility:

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

System:

NodeJS : v14.15.0
npm : 6.14.8
OS : macOS Big Sur

Additional Information

This is related to a previously reported issue but I have added more details to diagnose the problem.
#23435

This bug has been driving us crazy for a long time because the routing doesn't work and we were forced to use the same component for multiple routes to work around some other bugs.

One possible change that will improve the behavior of the component is to make the following change to path.ts.

The current code is:

export const chainToPath = (chain: RouteChain): string[] | null => {
  const path = [];
  for (const route of chain) {
    for (const segment of route.path) {
      if (segment[0] === ':') {
        const param = route.params && route.params[segment.slice(1)];
        if (!param) {
          return null;
        }
        path.push(param);
      } else if (segment !== '') {
        path.push(segment);
      }
    }
  }
  return path;
};

If the case for a param not found is changed to return the path that has matched so far (vs. null) and the routes are defined in order of most params to least params then it is possible to work around the problem.

// changes to path.ts
        if (!param) {
          return path;
        }

This allows it to return the portion of the path that it was able to resolve vs. returning null and resulting the the url not changing when clicking back.

A better solution would be to change the logic for the route matching to find the correct route based on the params but it is little harder to understand if that would impact other logic.

[liamdebeasi]

Thanks for the issue. Can you please reproduce this issue in an Ionic core starter app and provide a link to the repo?

[ionitron-bot]

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.

[cjorasch]

Created a repo that reproduces the issue.

https://github.com/cjorasch/issue-24223

To reproduce:

1. npm start
2. Click on router.push('a') button
3. Click on router.push('a/b') button
4. Click on router.push('a/b/c') button

There will now be 3 views in the stack

5. Click on nav.pop() button or click on the back arrow

The view will pop back to a/b but the url will change to / so the router and nav will be out of sync.

Additional details are available in the Javascript console.

[cjorasch]

Another related issue that I noticed is that the router has problems if you push content onto the nav stack using a component and props directly vs. something that maps to a url.

nav.push('my-content-page', { ... });

In this case the view is displayed but the router logs an error no matching URL for ['MY-CONTENT-PAGE'].

When you click the back button you pop to the prior view but the url is reset to /.

[sean-perkins]

@cjorasch thanks for the reported issue and extremely thorough analysis/replication! This indeed appears to be a bug with the logic used to find the best path match based on the navigation state.

I'll explore your suggested change to changeToPath as well as investigate alternative patterns to determine a match that takes navigation params into consideration.

We'll update you when we have something new to share 👍

[sean-perkins]

@cjorasch I have confirmed in your reproduction app that #24315 resolves the issue.

Can you install 5.10.0-dev.202112062007.8bcbbaa into your primary application and let me know if the issue is resolved there as well?

npm i @ionic/core@5.10.0-dev.202112062007.8bcbbaa

Thanks!

[cjorasch]

I am currently using the 6.0 branch so I am not able to test in our main app but I can take a look with the repro app to see if I see any issues.

Thanks!

[cjorasch]

I did some tests in the repro app and things looked like they are working correctly. I can test in our main app when it gets moved to the 6.0 branch.

[liamdebeasi]

Great! This fix will also be in the next Ionic 6 RC. I am going to close this out since the original issue seems to be resolved. If you run into any errors with this fix please let us know. Thank you!

[ionitron-bot]

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.