Skip to content

Commit

Permalink
fix: infinite dev reloads when parallel route is treated a page entry (
Browse files Browse the repository at this point in the history
…#52061)

### What?
When there's a parallel route adjacent to a tree that has no page
component, it's treated as an invalid entry in `handleAppPing` during
dev HMR, which causes an infinite refresh cycle

### Why?
In #51413, an update was made to `next-app-loader` to support layout
files in parallel routes. Part of this change updated the parallel
segment matching logic to mark the parallel page entry as `[
'@parallel', [ 'page$' ] ]` rather than `[ '@parallel', 'page$' ]`.

This resulted in `handleAppPing` looking for the corresponding page
entry at `client@app@/@parallel/page$/page` (note the `PAGE_SEGMENT`
marker) rather than `client@app@/@parallel/page`, causing the path to be
marked invalid on HMR pings, and triggering an endless fastRefresh.

### How?
A simple patch to fix this would fix this is to update `getEntryKey` to
replace any `PAGE_SEGMENT`'s that leak into the entry which I did in
59a972f.

The other option that's currently implemented here is to only insert
PAGE_SEGMENT as an array in the scenario where there isn't a page
adjacent to the parallel segment. This is to ensure that the
`parallelKey` is `children` rather than the `@parallel` slot when in
[`createSubtreePropsFromSegmentPath`](https://github.com/vercel/next.js/blob/59a972f53339cf6e444e3bf5be45bf115a24c31a/packages/next/src/build/webpack/loaders/next-app-loader.ts#L298).
This seems to not cause any regressions with the issue being fixed in
51413, and also solves this case, but I'm just not 100% sure if this
might break another scenario that I'm not thinking of.

Closes NEXT-1337
Fixes #51951
  • Loading branch information
ztanner committed Jul 4, 2023
1 parent 9e4b87d commit 0b87ba2
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 5 deletions.
13 changes: 9 additions & 4 deletions packages/next/src/build/webpack/loaders/next-app-loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,7 @@ async function createTreeCodeFromPath(
)}), ${JSON.stringify(resolvedPagePath)}],
${createMetadataExportsCode(metadata)}
}]`

continue
}

Expand Down Expand Up @@ -487,11 +488,15 @@ const nextAppLoader: AppLoader = async function nextAppLoader() {
}

const isParallelRoute = rest[0].startsWith('@')
if (isParallelRoute && rest.length === 2 && rest[1] === 'page') {
matched[rest[0]] = [PAGE_SEGMENT]
continue
}
if (isParallelRoute) {
if (rest.length === 2 && rest[1] === 'page') {
// matched will be an empty object in case the parallel route is at a path with no existing page
// in which case, we need to mark it as a regular page segment
matched[rest[0]] = Object.keys(matched).length
? [PAGE_SEGMENT]
: PAGE_SEGMENT
continue
}
// we insert a special marker in order to also process layout/etc files at the slot level
matched[rest[0]] = [PARALLEL_CHILDREN_SEGMENT, ...rest.slice(1)]
continue
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export default function ParallelPage() {
return (
<>
<p>Hello from parallel page!</p>
<div id="timestamp">{Date.now()}</div>
</>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import * as React from 'react'

export default function Layout({ parallel }: { parallel: React.ReactNode }) {
return (
<>
<h2>LAYOUT</h2>
{parallel}
</>
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ createNextDescribe(
{
files: __dirname,
},
({ next }) => {
({ next, isNextDev }) => {
describe('parallel routes', () => {
it('should support parallel route tab bars', async () => {
const browser = await next.browser('/parallel-tab-bar')
Expand Down Expand Up @@ -338,6 +338,23 @@ createNextDescribe(
`{"slug":"foo","id":"bar"}`
)
})

if (isNextDev) {
it('should support parallel routes with no page component', async () => {
const browser = await next.browser('/parallel-no-page/foo')
const timestamp = await browser.elementByCss('#timestamp').text()

await new Promise((resolve) => {
setTimeout(resolve, 3000)
})

await check(async () => {
// an invalid response triggers a fast refresh, so if the timestamp doesn't update, this behaved correctly
const newTimestamp = await browser.elementByCss('#timestamp').text()
return newTimestamp !== timestamp ? 'failure' : 'success'
}, 'success')
})
}
})

describe('route intercepting with dynamic routes', () => {
Expand Down

0 comments on commit 0b87ba2

Please sign in to comment.