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

fix: HMR not works when assetPrefix set to URL #68622

Merged
merged 6 commits into from
Aug 8, 2024
Merged

fix: HMR not works when assetPrefix set to URL #68622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3c0c5e4
test: add e2e test for assetPrefix with full url
devjiwonchoi Aug 7, 2024
63a8400
chore: add warn if assetprefix is url in dev
devjiwonchoi Aug 7, 2024
5f9da29
test: move to development
devjiwonchoi Aug 7, 2024
e862790
fix: check for hmr only for pathname
devjiwonchoi Aug 7, 2024
9a770f8
test: update to use createNext
devjiwonchoi Aug 8, 2024
d691ea9
misc: better comment
devjiwonchoi Aug 8, 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
10 changes: 10 additions & 0 deletions packages/next/src/server/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1069,6 +1069,16 @@ export default async function loadConfig(
}
}

if (
phase === PHASE_DEVELOPMENT_SERVER &&
URL.canParse(userConfig.assetPrefix ?? '')
) {
curLog.warn(
`Absolute URL assetPrefix "${userConfig.assetPrefix}" may disrupt development HMR.\n` +
'See more info here https://nextjs.org/docs/app/api-reference/next-config-js/assetPrefix'
)
}

if (userConfig.target && userConfig.target !== 'server') {
throw new Error(
`The "target" property is no longer supported in ${configFileName}.\n` +
Expand Down
8 changes: 8 additions & 0 deletions packages/next/src/server/lib/router-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,7 +668,15 @@ export async function initialize(opts: {
// assetPrefix overrides basePath for HMR path
if (assetPrefix) {
hmrPrefix = normalizedAssetPrefix(assetPrefix)

if (URL.canParse(hmrPrefix)) {
// remove trailing slash from pathname
// return empty string if pathname is '/'
// to avoid conflicts with '/_next' below
hmrPrefix = new URL(hmrPrefix).pathname.replace(/\/$/, '')
}
}

const isHMRRequest = req.url.startsWith(
ensureLeadingSlash(`${hmrPrefix}/_next/webpack-hmr`)
)
Comment on lines 680 to 682
Copy link
Member Author

Choose a reason for hiding this comment

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

Previous Behavior:

The HMR was broken and content was not updated.
Because we check for req.url (which starts with /path/name), and it would never set isHMRRequest to true.
It is because if the assetPrefix is http://localhost:3000, hmrPrefix would also be http://localhost:3000 and it can never match the pathname-style req.url

Expand Down
7 changes: 7 additions & 0 deletions test/development/app-dir/hmr-asset-prefix-full-url/app/layout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export default function Root({ children }) {
return (
<html>
<body>{children}</body>
</html>
)
}
3 changes: 3 additions & 0 deletions test/development/app-dir/hmr-asset-prefix-full-url/app/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <p>before edit</p>
}
32 changes: 32 additions & 0 deletions test/development/app-dir/hmr-asset-prefix-full-url/asset-prefix.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { createNext } from 'e2e-utils'
import { findPort, retry } from 'next-test-utils'

describe('app-dir assetPrefix full URL', () => {
let next, forcedPort
beforeAll(async () => {
forcedPort = ((await findPort()) ?? '54321').toString()

next = await createNext({
files: __dirname,
forcedPort,
nextConfig: {
assetPrefix: `http://localhost:${forcedPort}`,
},
})
})
afterAll(() => next.destroy())

it('should not break HMR when asset prefix set to full URL', async () => {
const browser = await next.browser('/')
const text = await browser.elementByCss('p').text()
expect(text).toBe('before edit')

await next.patchFile('app/page.tsx', (content) => {
return content.replace('before', 'after')
})

await retry(async () => {
expect(await browser.elementByCss('p').text()).toContain('after edit')
Copy link
Member Author

Choose a reason for hiding this comment

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

should be toBe

})
})
})
Loading