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 hydration mismatch on href for url with anchor refs #21065

Merged
merged 6 commits into from Jan 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions .github/ISSUE_TEMPLATE/1.bug_report.yml
Expand Up @@ -12,10 +12,10 @@ inputs:
value: If you leave out sections there is a high likelihood it will be moved to the GitHub Discussions "Help" section.
- type: description
attributes:
value: "Please first verify if your issue exists in the Next.js canary release line: `npm install next@canary`."
value: 'Please first verify if your issue exists in the Next.js canary release line: `npm install next@canary`.'
- type: description
attributes:
value: "next@canary is the beta version of Next.js. It includes all features and fixes that are pending to land on the stable release line."
value: 'next@canary is the beta version of Next.js. It includes all features and fixes that are pending to land on the stable release line.'
- type: input
attributes:
label: What version of Next.js are you using?
Expand Down
3 changes: 2 additions & 1 deletion packages/next/next-server/lib/router/router.ts
Expand Up @@ -163,7 +163,8 @@ export function delBasePath(path: string): string {
* Detects whether a given url is routable by the Next.js router (browser only).
*/
export function isLocalURL(url: string): boolean {
if (url.startsWith('/')) return true
// prevent a hydration mismatch on href for url with anchor refs
if (url.startsWith('/') || url.startsWith('#')) return true
try {
// absolute urls can be local if they are on the same origin
const locationOrigin = getLocationOrigin()
Expand Down
14 changes: 14 additions & 0 deletions test/integration/link-with-hash/pages/index.js
@@ -0,0 +1,14 @@
import React from 'react'
import Link from 'next/link'

const Home = () => {
return (
<>
<Link href="#hash-link">
<a>Hash Link</a>
</Link>
</>
)
}

export default Home
53 changes: 53 additions & 0 deletions test/integration/link-with-hash/test/index.test.js
@@ -0,0 +1,53 @@
/* eslint-env jest */

import { join } from 'path'
import webdriver from 'next-webdriver'
import {
findPort,
launchApp,
killApp,
nextStart,
nextBuild,
} from 'next-test-utils'

jest.setTimeout(1000 * 60 * 5)
let app
let appPort
const appDir = join(__dirname, '..')

const runTests = () => {
it('should not have hydration mis-match for hash link', async () => {
const browser = await webdriver(appPort, '/')
const browserLogs = await browser.log('browser')
let found = false
browserLogs.forEach((log) => {
if (log.message.includes('Warning: Prop')) {
Copy link
Member

Choose a reason for hiding this comment

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

Hi, this warning won't show in production mode so we don't need to test that, we could add this check to the client-navigation suite here using the /nav/hash-changes page to keep the test related to that. The check looks good besides that!

Copy link
Contributor Author

@kaykdm kaykdm Jan 16, 2021

Choose a reason for hiding this comment

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

@ijjk (cc: @timneutkens )
Hi, thanks for the review!
You are right that this warning does not show in production.
I was not sure where to put this test, so I created new one. Adding this check to the client-navigation suite using the /nav/hash-changes makes sense to me.
Since this PR is merged, should I make an another PR to apply your suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

If you'd like to create another PR updating the test that would be awesome!

found = true
}
})
expect(found).toEqual(false)
})
}

describe('Link with hash href', () => {
describe('development', () => {
beforeAll(async () => {
appPort = await findPort()
app = await launchApp(appDir, appPort)
})
afterAll(() => killApp(app))

runTests()
})

describe('production', () => {
beforeAll(async () => {
await nextBuild(appDir)
appPort = await findPort()
app = await nextStart(appDir, appPort)
})
afterAll(() => killApp(app))

runTests()
})
})