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.

Sign up for GitHub

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

Jump to bottom

fix(vue): incorrect view rendered when using router.go(-n) #29877

Merged
merged 3 commits into from
Oct 22, 2024
Merged

fix(vue): incorrect view rendered when using router.go(-n) #29877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4141703
Add tests for finding last location
bentleyo Sep 17, 2024
f03cc5a
Fix findLastLocation so that deltas less than -1 are still relative t…
bentleyo Sep 17, 2024
90b09ba
Simplify
bentleyo Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions packages/vue-router/__tests__/locationHistory.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,25 @@ describe('Location History', () => {
expect(locationHistory.canGoBack(1, 0, 1)).toEqual(true);
expect(locationHistory.canGoBack(2, 0, 1)).toEqual(false);
});

it('should correctly find the last location', () => {
const [home, pageA, pageB, pageC] = [
{ pathname: '/home' },
{ pathname: '/page-a', pushedByRoute: '/home' },
{ pathname: '/page-b', pushedByRoute: '/page-a' },
{ pathname: '/page-c', pushedByRoute: '/page-b' },
];

locationHistory.add(home);
locationHistory.add(pageA);
locationHistory.add(pageB);
locationHistory.add(pageC);

expect(locationHistory.findLastLocation(pageB)).toEqual(pageA);
expect(locationHistory.findLastLocation(pageB, -2)).toEqual(home);

expect(locationHistory.findLastLocation(pageC)).toEqual(pageB);
expect(locationHistory.findLastLocation(pageC, -2)).toEqual(pageA);
expect(locationHistory.findLastLocation(pageC, -3)).toEqual(home);
});
});
14 changes: 5 additions & 9 deletions packages/vue-router/src/locationHistory.ts
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for submitting a PR!

I noticed you removed the if statement from the code. I'd love to understand your reasoning behind that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @thetaPC thanks for the review!

The if statement I've removed was originally added to handle the situation where delta is less than -1. In that situation what it does is return the location delta many steps back from the end of the history stack.

if (delta < -1) {
  return locationHistory[locationHistory.length - 1 + delta];
}

However, this wasn't correct behaviour. There is a routeInfo passed into the function and the returned location should be delta steps back from that. Not back from the end of locationHistory.

This is why the bug reported in the issue was happening.

I've made it so it handles both situations using that same code. This is mostly reusing the code that was in the else statement.

That code essentially searches backwards through the history stack until it finds routeInfo and returns its parent (handling the delta -1 situation). I realised I could use index of that location in the array + delta to return the correct location.

I've added tests to handle the situations and ensure the behaviour works as expected. Let me know if you need any more details :)

Original file line number Diff line number Diff line change
Expand Up @@ -239,15 +239,11 @@ export const createLocationHistory = () => {
}
}
}
if (delta < -1) {
return locationHistory[locationHistory.length - 1 + delta];
} else {
for (let i = locationHistory.length - 2; i >= 0; i--) {
const ri = locationHistory[i];
if (ri) {
if (ri.pathname === routeInfo.pushedByRoute) {
return ri;
}
for (let i = locationHistory.length - 2; i >= 0; i--) {
const ri = locationHistory[i];
if (ri) {
if (ri.pathname === routeInfo.pushedByRoute) {
return locationHistory[i + 1 + delta]
}
}
}
Expand Down
Loading