Skip to content

Commit

Permalink
Fix disposing active entries in dev compilers (#39845)
Browse files Browse the repository at this point in the history
As noticed in markdoc/markdoc#131 it seems we are incorrectly disposing active entries causing HMR to fail after the configured `maxInactiveAge`. To fix this instead of only updating lastActiveTime for one compiler type's entry we update all active compiler types for the active entry. 

This also updates an existing test to catch this by waiting the `maxInactiveAge` before attempting a change that should trigger HMR. 

## Bug

- [x] Related issues linked using `fixes #number`
- [x] Integration tests added
- [x] Errors have helpful link attached, see `contributing.md`

Fixes: markdoc/markdoc#131
  • Loading branch information
ijjk committed Aug 23, 2022
1 parent 31bcd04 commit 611e13f
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 40 deletions.
96 changes: 57 additions & 39 deletions packages/next/server/dev/on-demand-entry-handler.ts
Expand Up @@ -453,65 +453,83 @@ export function onDemandEntryHandler({
tree: FlightRouterState
): { success: true } | { invalid: true } {
const pages = getEntrypointsFromTree(tree, true)
let toSend: { invalid: true } | { success: true } = { invalid: true }

for (const page of pages) {
const pageKey = `server/${page}`
for (const compilerType of [
COMPILER_NAMES.client,
COMPILER_NAMES.server,
COMPILER_NAMES.edgeServer,
]) {
const pageKey = `${compilerType}/${page}`
const entryInfo = entries[pageKey]

// If there's no entry, it may have been invalidated and needs to be re-built.
if (!entryInfo) {
// if (page !== lastEntry) client pings, but there's no entry for page
continue
}

// We don't need to maintain active state of anything other than BUILT entries
if (entryInfo.status !== BUILT) continue

// If there's an entryInfo
if (!lastServerAccessPagesForAppDir.includes(pageKey)) {
lastServerAccessPagesForAppDir.unshift(pageKey)

// Maintain the buffer max length
// TODO: verify that the current pageKey is not at the end of the array as multiple entrypoints can exist
if (lastServerAccessPagesForAppDir.length > pagesBufferLength) {
lastServerAccessPagesForAppDir.pop()
}
}
entryInfo.lastActiveTime = Date.now()
entryInfo.dispose = false
toSend = { success: true }
}
}
return toSend
}

function handlePing(pg: string) {
const page = normalizePathSep(pg)
let toSend: { invalid: true } | { success: true } = { invalid: true }

for (const compilerType of [
COMPILER_NAMES.client,
COMPILER_NAMES.server,
COMPILER_NAMES.edgeServer,
]) {
const pageKey = `${compilerType}${page}`
const entryInfo = entries[pageKey]

// If there's no entry, it may have been invalidated and needs to be re-built.
if (!entryInfo) {
// if (page !== lastEntry) client pings, but there's no entry for page
return { invalid: true }
if (compilerType === COMPILER_NAMES.client) {
return { invalid: true }
}
continue
}

// 404 is an on demand entry but when a new page is added we have to refresh the page
toSend = page === '/_error' ? { invalid: true } : { success: true }

// We don't need to maintain active state of anything other than BUILT entries
if (entryInfo.status !== BUILT) continue

// If there's an entryInfo
if (!lastServerAccessPagesForAppDir.includes(pageKey)) {
lastServerAccessPagesForAppDir.unshift(pageKey)
if (!lastClientAccessPages.includes(pageKey)) {
lastClientAccessPages.unshift(pageKey)

// Maintain the buffer max length
// TODO: verify that the current pageKey is not at the end of the array as multiple entrypoints can exist
if (lastServerAccessPagesForAppDir.length > pagesBufferLength) {
lastServerAccessPagesForAppDir.pop()
if (lastClientAccessPages.length > pagesBufferLength) {
lastClientAccessPages.pop()
}
}
entryInfo.lastActiveTime = Date.now()
entryInfo.dispose = false
}

return { success: true }
}

function handlePing(pg: string) {
const page = normalizePathSep(pg)
const pageKey = `client${page}`
const entryInfo = entries[pageKey]

// If there's no entry, it may have been invalidated and needs to be re-built.
if (!entryInfo) {
// if (page !== lastEntry) client pings, but there's no entry for page
return { invalid: true }
}

// 404 is an on demand entry but when a new page is added we have to refresh the page
const toSend = page === '/_error' ? { invalid: true } : { success: true }

// We don't need to maintain active state of anything other than BUILT entries
if (entryInfo.status !== BUILT) return

// If there's an entryInfo
if (!lastClientAccessPages.includes(pageKey)) {
lastClientAccessPages.unshift(pageKey)

// Maintain the buffer max length
if (lastClientAccessPages.length > pagesBufferLength) {
lastClientAccessPages.pop()
}
}
entryInfo.lastActiveTime = Date.now()
entryInfo.dispose = false
return toSend
}

Expand Down
Expand Up @@ -3,7 +3,7 @@
import { join } from 'path'
import webdriver from 'next-webdriver'
import { createNext, FileRef } from 'e2e-utils'
import { check, getRedboxHeader, hasRedbox } from 'next-test-utils'
import { check, getRedboxHeader, hasRedbox, waitFor } from 'next-test-utils'
import { NextInstance } from 'test/lib/next-modes/base'

const installCheckVisible = (browser) => {
Expand Down Expand Up @@ -319,6 +319,10 @@ describe('GS(S)P Server-Side Change Reloading', () => {
expect(props.count).toBe(1)
expect(props.data).toEqual({ hello: 'world' })

// wait longer than the max inactive age for on-demand entries
// to ensure we aren't incorrectly disposing the active entry
await waitFor(20 * 1000)

const page = 'lib/data.json'
const originalContent = await next.readFile(page)

Expand Down

0 comments on commit 611e13f

Please sign in to comment.