Skip to content

misc: tweak fetch patch restoration timing during HMR to allow for userland fetch patching #68193

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

Merged
merged 8 commits into from
Aug 20, 2024
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/next/src/server/dev/hot-reloader-turbopack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ const sessionId = Math.floor(Number.MAX_SAFE_INTEGER * Math.random())
export async function createHotReloaderTurbopack(
opts: SetupOpts,
serverFields: ServerFields,
distDir: string
distDir: string,
resetFetch: () => void
): Promise<NextJsHotReloaderInterface> {
const buildId = 'development'
const { nextConfig, dir } = opts
Expand Down Expand Up @@ -236,6 +237,8 @@ export async function createHotReloaderTurbopack(
}
}

resetFetch()

const hasAppPaths = writtenEndpoint.serverPaths.some(({ path: p }) =>
p.startsWith('server/app')
)
Expand Down
5 changes: 5 additions & 0 deletions packages/next/src/server/dev/hot-reloader-webpack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ export default class HotReloaderWebpack implements NextJsHotReloaderInterface {
private pagesMapping: { [key: string]: string } = {}
private appDir?: string
private telemetry: Telemetry
private resetFetch: () => void
private versionInfo: VersionInfo = {
staleness: 'unknown',
installed: '0.0.0',
Expand All @@ -274,6 +275,7 @@ export default class HotReloaderWebpack implements NextJsHotReloaderInterface {
rewrites,
appDir,
telemetry,
resetFetch,
}: {
config: NextConfigComplete
pagesDir?: string
Expand All @@ -284,6 +286,7 @@ export default class HotReloaderWebpack implements NextJsHotReloaderInterface {
rewrites: CustomRoutes['rewrites']
appDir?: string
telemetry: Telemetry
resetFetch: () => void
}
) {
this.hasAmpEntrypoints = false
Expand All @@ -301,6 +304,7 @@ export default class HotReloaderWebpack implements NextJsHotReloaderInterface {
this.edgeServerStats = null
this.serverPrevDocumentHash = null
this.telemetry = telemetry
this.resetFetch = resetFetch

this.config = config
this.previewProps = previewProps
Expand Down Expand Up @@ -1365,6 +1369,7 @@ export default class HotReloaderWebpack implements NextJsHotReloaderInterface {
changedCSSImportPages.size ||
reloadAfterInvalidation
) {
this.resetFetch()
Copy link
Member

Choose a reason for hiding this comment

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

Can we have some comments about why we only conditionally reset fetch instead of doing somewhere will always get running?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because this branch checks for server components changes. I can add a comment tho

this.refreshServerComponents()
}

Expand Down
17 changes: 0 additions & 17 deletions packages/next/src/server/dev/next-dev-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,6 @@ export default class DevServer extends Server {
this.bundlerService = options.bundlerService
this.startServerSpan =
options.startServerSpan ?? trace('start-next-dev-server')
this.storeGlobals()
this.renderOpts.dev = true
this.renderOpts.ErrorDebug = ReactDevOverlay
this.staticPathsCache = new LRUCache({
Expand Down Expand Up @@ -294,9 +293,6 @@ export default class DevServer extends Server {
await super.prepareImpl()
await this.matchers.reload()

// Store globals again to preserve changes made by the instrumentation hook.
this.storeGlobals()

this.ready?.resolve()
this.ready = undefined

Expand Down Expand Up @@ -825,14 +821,6 @@ export default class DevServer extends Server {
return nextInvoke as NonNullable<typeof result>
}

private storeGlobals(): void {
this.originalFetch = global.fetch
}

private restorePatchedGlobals(): void {
global.fetch = this.originalFetch ?? global.fetch
}

protected async ensurePage(opts: {
page: string
clientOnly: boolean
Expand Down Expand Up @@ -880,11 +868,6 @@ export default class DevServer extends Server {
}

this.nextFontManifest = super.getNextFontManifest()
// before we re-evaluate a route module, we want to restore globals that might
// have been patched previously to their original state so that we don't
// patch on top of the previous patch, which would keep the context of the previous
// patched global in memory, creating a memory leak.
this.restorePatchedGlobals()

return await super.findPageComponents({
page,
Expand Down
15 changes: 9 additions & 6 deletions packages/next/src/server/lib/patch-fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@ type PatchedFetcher = Fetcher & {
readonly _nextOriginalFetch: Fetcher
}

function isPatchedFetch(
fetch: Fetcher | PatchedFetcher
): fetch is PatchedFetcher {
return '__nextPatched' in fetch && fetch.__nextPatched === true
export const NEXT_PATCH_SYMBOL = Symbol.for('next-patch')

function isFetchPatched() {
return (globalThis as Record<symbol, unknown>)[NEXT_PATCH_SYMBOL] === true
}

export function validateRevalidate(
Expand Down Expand Up @@ -804,18 +804,21 @@ export function createPatchedFetcher(
}

// Attach the necessary properties to the patched fetch function.
// We don't use this to determine if the fetch function has been patched,
// but for external consumers to determine if the fetch function has been
// patched.
patched.__nextPatched = true as const
patched.__nextGetStaticStore = () => staticGenerationAsyncStorage
patched._nextOriginalFetch = originFetch
;(globalThis as Record<symbol, unknown>)[NEXT_PATCH_SYMBOL] = true

return patched
}

// we patch fetch to collect cache information used for
// determining if a page is static or not
export function patchFetch(options: PatchableModule) {
// If we've already patched fetch, we should not patch it again.
if (isPatchedFetch(globalThis.fetch)) return
if (isFetchPatched()) return

// Grab the original fetch function. We'll attach this so we can use it in
// the patched fetch function.
Expand Down
17 changes: 13 additions & 4 deletions packages/next/src/server/lib/router-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import {
type AppIsrManifestAction,
} from '../dev/hot-reloader-types'
import { normalizedAssetPrefix } from '../../shared/lib/normalized-asset-prefix'
import { NEXT_PATCH_SYMBOL } from './patch-fetch'

const debug = setupDebug('next:router-server:main')
const isNextFont = (pathname: string | null) =>
Expand Down Expand Up @@ -109,6 +110,8 @@ export async function initialize(opts: {

let devBundlerService: DevBundlerService | undefined

let originalFetch = globalThis.fetch

if (opts.dev) {
const { Telemetry } =
require('../../telemetry/storage') as typeof import('../../telemetry/storage')
Expand All @@ -121,6 +124,11 @@ export async function initialize(opts: {
const { setupDevBundler } =
require('./router-utils/setup-dev-bundler') as typeof import('./router-utils/setup-dev-bundler')

const resetFetch = () => {
globalThis.fetch = originalFetch
;(globalThis as Record<symbol, unknown>)[NEXT_PATCH_SYMBOL] = false
}

const setupDevBundlerSpan = opts.startServerSpan
? opts.startServerSpan.traceChild('setup-dev-bundler')
: trace('setup-dev-bundler')
Expand All @@ -138,6 +146,7 @@ export async function initialize(opts: {
turbo: !!process.env.TURBOPACK,
port: opts.port,
onCleanup: opts.onCleanup,
resetFetch,
})
)

Expand Down Expand Up @@ -591,12 +600,12 @@ export async function initialize(opts: {
let requestHandler: WorkerRequestHandler = requestHandlerImpl
if (config.experimental.testProxy) {
// Intercept fetch and other testmode apis.
const {
wrapRequestHandlerWorker,
interceptTestApis,
} = require('next/dist/experimental/testmode/server')
const { wrapRequestHandlerWorker, interceptTestApis } =
require('next/dist/experimental/testmode/server') as typeof import('next/src/experimental/testmode/server')
requestHandler = wrapRequestHandlerWorker(requestHandler)
interceptTestApis()
// We treat the intercepted fetch as "original" fetch that should be reset to during HMR.
originalFetch = globalThis.fetch
}
requestHandlers[opts.dir] = requestHandler

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ export type SetupOpts = {
nextConfig: NextConfigComplete
port: number
onCleanup: (listener: () => Promise<void>) => void
resetFetch: () => void
}

export type ServerFields = {
Expand All @@ -122,6 +123,7 @@ export type ServerFields = {
typeof import('./filesystem').buildCustomRoute
>[]
setAppIsrStatus?: (key: string, value: false | number | null) => void
resetFetch?: () => void
}

async function verifyTypeScript(opts: SetupOpts) {
Expand Down Expand Up @@ -152,7 +154,7 @@ export async function propagateServerField(
}

async function startWatcher(opts: SetupOpts) {
const { nextConfig, appDir, pagesDir, dir } = opts
const { nextConfig, appDir, pagesDir, dir, resetFetch } = opts
const { useFileSystemPublicRoutes } = nextConfig
const usingTypeScript = await verifyTypeScript(opts)

Expand Down Expand Up @@ -182,7 +184,7 @@ async function startWatcher(opts: SetupOpts) {
})

const hotReloader: NextJsHotReloaderInterface = opts.turbo
? await createHotReloaderTurbopack(opts, serverFields, distDir)
? await createHotReloaderTurbopack(opts, serverFields, distDir, resetFetch)
: new HotReloaderWebpack(opts.dir, {
appDir,
pagesDir,
Expand All @@ -193,6 +195,7 @@ async function startWatcher(opts: SetupOpts) {
telemetry: opts.telemetry,
rewrites: opts.fsChecker.rewrites,
previewProps: opts.fsChecker.prerenderManifest.preview,
resetFetch,
})

await hotReloader.start()
Expand Down
36 changes: 36 additions & 0 deletions test/development/app-dir/dev-fetch-hmr/app/layout.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import React from 'react'
import { ReactNode } from 'react'

const magicNumber = Math.random()
const originalFetch = globalThis.fetch

if (originalFetch.name === 'monkeyPatchedFetch') {
throw new Error(
'Patching over already patched fetch. This creates a memory leak.'
)
}

globalThis.fetch = async function monkeyPatchedFetch(
resource: URL | RequestInfo,
options?: RequestInit
) {
const request = new Request(resource)

if (request.url === 'http://fake.url/secret') {
return new Response('monkey patching is fun')
}

if (request.url === 'http://fake.url/magic-number') {
return new Response(magicNumber.toString())
}

return originalFetch(resource, options)
}

export default function Root({ children }: { children: ReactNode }) {
return (
<html>
<body>{children}</body>
</html>
)
}
16 changes: 16 additions & 0 deletions test/development/app-dir/dev-fetch-hmr/app/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
export default async function Page() {
const secret = (await fetch('http://fake.url/secret').then((res) =>
res.text()
)) as any
const magicNumber = (await fetch('http://fake.url/magic-number').then((res) =>
res.text()
)) as any

return (
<>
<div id="update">touch to trigger HMR</div>
<div id="secret">{secret}</div>
<div id="magic-number">{magicNumber}</div>
</>
)
}
40 changes: 40 additions & 0 deletions test/development/app-dir/dev-fetch-hmr/dev-fetch-hmr.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { nextTestSetup } from 'e2e-utils'
import { retry } from 'next-test-utils'

import cheerio from 'cheerio'

describe('dev-fetch-hmr', () => {
const { next } = nextTestSetup({
files: __dirname,
})

it('should retain module level fetch patching', async () => {
const html = await next.render('/')
expect(html).toContain('monkey patching is fun')

const magicNumber = cheerio.load(html)('#magic-number').text()

const html2 = await next.render('/')
expect(html2).toContain('monkey patching is fun')
const magicNumber2 = cheerio.load(html2)('#magic-number').text()
// Module was not re-evaluated
expect(magicNumber2).toBe(magicNumber)
const update = cheerio.load(html2)('#update').text()
expect(update).toBe('touch to trigger HMR')

// trigger HMR
await next.patchFile('app/page.tsx', (content) =>
content.replace('touch to trigger HMR', 'touch to trigger HMR 2')
)

await retry(async () => {
const html3 = await next.render('/')
const update2 = cheerio.load(html3)('#update').text()
expect(update2).toBe('touch to trigger HMR 2')
const magicNumber3 = cheerio.load(html3)('#magic-number').text()
expect(html3).toContain('monkey patching is fun')
// Module was re-evaluated
expect(magicNumber3).not.toEqual(magicNumber)
})
})
})
6 changes: 6 additions & 0 deletions test/development/app-dir/dev-fetch-hmr/next.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/**
* @type {import('next').NextConfig}
*/
const nextConfig = {}

module.exports = nextConfig
24 changes: 24 additions & 0 deletions test/development/app-dir/dev-fetch-hmr/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
{
"compilerOptions": {
"target": "ES2017",
"lib": ["dom", "dom.iterable", "esnext"],
"allowJs": true,
"skipLibCheck": true,
"strict": false,
"noEmit": true,
"incremental": true,
"module": "esnext",
"esModuleInterop": true,
"moduleResolution": "node",
"resolveJsonModule": true,
"isolatedModules": true,
"jsx": "preserve",
"plugins": [
{
"name": "next"
}
]
},
"include": ["next-env.d.ts", ".next/types/**/*.ts", "**/*.ts", "**/*.tsx"],
"exclude": ["node_modules"]
}
Loading