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

fix(vue): replacing routes now updates location state correctly #24721

Merged
merged 7 commits into from
Feb 7, 2022

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Feb 4, 2022

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: resolves #24432

There are a few related issues:

  1. When the incoming route had routeAction === 'replace' Ionic Vue would determine the leaving location by getting the second to last item in location history. This was fine when popping would remove items from location history automatically (I.e. what Ionic React does), but when you support router.go(n) you cannot do that. As a result, the locationHistory.previous() call was not always returning the correct previous route based on where the user was in location history.

  2. The routeInfo.tab === undefined check was actually a bandaid for an issue I had run into when calling the old updateByHistoryPosition method (See:

    locationHistory.updateByHistoryPosition(routeInfo, !hasDeltaStride);
    ). However, it was later determined in 0b18260 that this was not the correct solution for the problem that it tried to solve. Now that we have the correct solution in place, this bandaid is no longer needed.

  3. When re-writing history, Ionic Vue never considered the case where you wanted to replace the current state of the route with another route and as a result, the location history was not being updated/wiped appropriately. This worked fine when you navigated backwards and then did a router.replace, but if you just did a router.replace on its own, location history did not update properly.

What is the new behavior?

  • Removed the .previous method in favor of calling .current which takes into account your location within history. I also removed the previous method in general as it is no longer used in the codebase.
  • Removed the routeInfo.tab bandaid.
  • Added a check to account for when doing router.replace in order to determine if we should wipe the location history.
  • Unrelated to the PR, but there was a stray cy.wait that was not needed. I removed that.

Does this introduce a breaking change?

  • Yes
  • No

Other information

@liamdebeasi liamdebeasi requested a review from a team as a code owner February 4, 2022 17:26
@github-actions github-actions bot added the package: vue @ionic/vue package label Feb 4, 2022
@liamdebeasi
Copy link
Contributor Author

One thing I am still trying to figure out is if we should be clearing out the tabs history at all when we call clearHistory? https://github.com/ionic-team/ionic-framework/blob/main/packages/vue-router/src/router.ts#L358

When clearing out location history, maybe we could get a list of the cleared routes and remove those same routes from the tabs history? Might be a bit of a performance overhead, but I worry that simply clearing out tabs history will cause unexpected issues down the road.

Going to investigate this more prior to merge, but let me know if you have any thoughts.

Copy link
Contributor

@amandaejohnston amandaejohnston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM 👍 Re: clearing out tabs, I'm hesitant to touch more than needed at once with router stuff; LMK if your investigation turns up anything but I'd want a strong repro/use case before forming opinions.

@liamdebeasi
Copy link
Contributor Author

Agreed. I couldn't think of a use case where this is relevant. I am in favor of leaving it alone for now until we get feedback from the community.

@sean-perkins sean-perkins self-requested a review February 7, 2022 18:30
Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Agreed the discussion regarding leaving tabs alone for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: vue @ionic/vue package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: router.replace eventually breaks routing
3 participants