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

Don't pushState when already on the url #42735

Merged
merged 2 commits into from Nov 10, 2022
Merged

Don't pushState when already on the url #42735

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f50a651
Don't pushState when already on the url
timneutkens Nov 10, 2022
2738959
Merge branch 'canary' into fix/additional-push-bug
kodiakhq[bot] Nov 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/next/client/components/app-router.tsx
View file
Open in desktop
Expand Up @@ -20,6 +20,7 @@ import {
ACTION_REFRESH,
ACTION_RESTORE,
ACTION_SERVER_PATCH,
createHrefFromUrl,
reducer,
} from './reducer'
import {
Expand Down Expand Up @@ -291,7 +292,10 @@ function Router({
// __NA is used to identify if the history entry can be handled by the app-router.
// __N is used to identify if the history entry can be handled by the old router.
const historyState = { __NA: true, tree }
if (pushRef.pendingPush) {
if (
pushRef.pendingPush &&
createHrefFromUrl(new URL(window.location.href)) !== canonicalUrl
Copy link
Contributor

Choose a reason for hiding this comment

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

Since window.location is a Location object that has all the same fields as URL, maybe we can just use that instead of turning it into a string, parsing it into a new object and then reassemble.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened a PR here: #42888

) {
// This intentionally mutates React state, pushRef is overwritten to ensure additional push/replace calls do not trigger an additional history entry.
pushRef.pendingPush = false

Expand Down
2 changes: 1 addition & 1 deletion packages/next/client/components/reducer.ts
View file
Open in desktop
Expand Up @@ -43,7 +43,7 @@ function readRecordValue(thenable: any) {
}
}

function createHrefFromUrl(url: URL): string {
export function createHrefFromUrl(url: URL): string {
return url.pathname + url.search + url.hash
}

Expand Down
23 changes: 23 additions & 0 deletions test/e2e/app-dir/index.test.ts
View file
Open in desktop
Expand Up @@ -2135,6 +2135,29 @@ describe('app dir', () => {
.text()
).toBe(`About page`)
})
it('should not do additional pushState when already on the page', async () => {
const browser = await webdriver(next.url, '/linking/about')
const goToLinkingPage = async () => {
expect(
await browser
.elementByCss('a[href="/linking"]')
.click()
.waitForElementByCss('#home-page')
.text()
).toBe(`Home page`)
}

await goToLinkingPage()
await waitFor(1000)
await goToLinkingPage()
await waitFor(1000)
await goToLinkingPage()
await waitFor(1000)

expect(
await browser.back().waitForElementByCss('#about-page', 2000).text()
).toBe(`About page`)
})
})

describe('not-found', () => {
Expand Down