Skip to content

Commit

Permalink
Fix missing cleanup process in flight plugin globals (#43297)
Browse files Browse the repository at this point in the history
This PR adds proper cleanup step for ~`edgeServerModuleIds`,
`serverModuleIds` and~ `flightCSSManifest`, which was previously
missing. Otherwise it will grow indefinitely during development.

## Bug

- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Errors have a helpful link attached, see
[`contributing.md`](https://github.com/vercel/next.js/blob/canary/contributing.md)

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the
feature request has been accepted for implementation before opening a
PR.
- [ ] Related issues linked using `fixes #number`
- [ ]
[e2e](https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs)
tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have a helpful link attached, see
[`contributing.md`](https://github.com/vercel/next.js/blob/canary/contributing.md)

## Documentation / Examples

- [ ] Make sure the linting passes by running `pnpm build && pnpm lint`
- [ ] The "examples guidelines" are followed from [our contributing
doc](https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md)

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
  • Loading branch information
shuding and kodiakhq[bot] committed Nov 29, 2022
1 parent fdff88b commit e816e45
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 54 deletions.
33 changes: 25 additions & 8 deletions packages/next/build/webpack/plugins/flight-client-entry-plugin.ts
Expand Up @@ -3,6 +3,8 @@ import type {
ClientComponentImports,
NextFlightClientEntryLoaderOptions,
} from '../loaders/next-flight-client-entry-loader'
import type { FlightCSSManifest } from './flight-manifest-plugin'

import { webpack } from 'next/dist/compiled/webpack/webpack'
import { stringify } from 'querystring'
import path from 'path'
Expand All @@ -19,7 +21,6 @@ import {
EDGE_RUNTIME_WEBPACK,
FLIGHT_SERVER_CSS_MANIFEST,
} from '../../../shared/lib/constants'
import { FlightCSSManifest } from './flight-manifest-plugin'
import { ASYNC_CLIENT_MODULES } from './flight-manifest-plugin'
import { isClientComponentModule, regexCSS } from '../loaders/utils'
import { traverseModules } from '../utils'
Expand All @@ -37,8 +38,8 @@ export const injectedClientEntries = new Map()
export const serverModuleIds = new Map<string, string | number>()
export const edgeServerModuleIds = new Map<string, string | number>()

// TODO-APP: move CSS manifest generation to the flight manifest plugin.
const flightCSSManifest: FlightCSSManifest = {}
let serverCSSManifest: FlightCSSManifest = {}
let edgeServerCSSManifest: FlightCSSManifest = {}

export class FlightClientEntryPlugin {
dev: boolean
Expand Down Expand Up @@ -112,7 +113,7 @@ export class FlightClientEntryPlugin {
}

traverseModules(compilation, (mod, _chunk, _chunkGroup, modId) => {
recordModule(modId + '', mod)
recordModule(String(modId), mod)
})
})
}
Expand Down Expand Up @@ -234,6 +235,15 @@ export class FlightClientEntryPlugin {
// by the certain chunk.
compilation.hooks.afterOptimizeModules.tap(PLUGIN_NAME, () => {
const cssImportsForChunk: Record<string, string[]> = {}
if (this.isEdgeServer) {
edgeServerCSSManifest = {}
} else {
serverCSSManifest = {}
}

let cssManifest = this.isEdgeServer
? edgeServerCSSManifest
: serverCSSManifest

function collectModule(entryName: string, mod: any) {
const resource = mod.resource
Expand Down Expand Up @@ -272,10 +282,10 @@ export class FlightClientEntryPlugin {
}

const entryCSSInfo: Record<string, string[]> =
flightCSSManifest.__entry_css__ || {}
cssManifest.__entry_css__ || {}
entryCSSInfo[entryName] = cssImportsForChunk[entryName]

Object.assign(flightCSSManifest, {
Object.assign(cssManifest, {
__entry_css__: entryCSSInfo,
})
})
Expand Down Expand Up @@ -320,7 +330,7 @@ export class FlightClientEntryPlugin {
clientEntryDependencyMap,
})

Object.assign(flightCSSManifest, cssImports)
Object.assign(cssManifest, cssImports)
}
})
})
Expand All @@ -333,7 +343,14 @@ export class FlightClientEntryPlugin {
stage: webpack.Compilation.PROCESS_ASSETS_STAGE_OPTIMIZE_HASH,
},
(assets: webpack.Compilation['assets']) => {
const manifest = JSON.stringify(flightCSSManifest)
const manifest = JSON.stringify({
...serverCSSManifest,
...edgeServerCSSManifest,
__entry_css__: {
...serverCSSManifest.__entry_css__,
...edgeServerCSSManifest.__entry_css__,
},
})
assets[FLIGHT_SERVER_CSS_MANIFEST + '.json'] = new sources.RawSource(
manifest
) as unknown as webpack.sources.RawSource
Expand Down
89 changes: 43 additions & 46 deletions test/e2e/app-dir/index.test.ts
Expand Up @@ -382,6 +382,49 @@ describe('app dir', () => {
}
}
)
;(isDev ? describe : describe.skip)('HMR', () => {
it('should HMR correctly for client component', async () => {
const filePath = 'app/client-component-route/page.js'
const origContent = await next.readFile(filePath)

try {
const browser = await webdriver(next.url, '/client-component-route')

const ssrInitial = await renderViaHTTP(
next.url,
'/client-component-route'
)

expect(ssrInitial).toContain('hello from app/client-component-route')

expect(await browser.elementByCss('p').text()).toContain(
'hello from app/client-component-route'
)

await next.patchFile(
filePath,
origContent.replace('hello from', 'swapped from')
)

await check(() => browser.elementByCss('p').text(), /swapped from/)

const ssrUpdated = await renderViaHTTP(
next.url,
'/client-component-route'
)
expect(ssrUpdated).toContain('swapped from')

await next.patchFile(filePath, origContent)

await check(() => browser.elementByCss('p').text(), /hello from/)
expect(
await renderViaHTTP(next.url, '/client-component-route')
).toContain('hello from')
} finally {
await next.patchFile(filePath, origContent)
}
})
})

it('should handle hash in initial url', async () => {
const browser = await webdriver(next.url, '/dashboard#abc')
Expand Down Expand Up @@ -1332,52 +1375,6 @@ describe('app dir', () => {
})
})
})

if (isDev) {
it('should HMR correctly for client component', async () => {
const filePath = 'app/client-component-route/page.js'
const origContent = await next.readFile(filePath)

try {
const browser = await webdriver(next.url, '/client-component-route')

const ssrInitial = await renderViaHTTP(
next.url,
'/client-component-route'
)

expect(ssrInitial).toContain(
'hello from app/client-component-route'
)

expect(await browser.elementByCss('p').text()).toContain(
'hello from app/client-component-route'
)

await next.patchFile(
filePath,
origContent.replace('hello from', 'swapped from')
)

await check(() => browser.elementByCss('p').text(), /swapped from/)

const ssrUpdated = await renderViaHTTP(
next.url,
'/client-component-route'
)
expect(ssrUpdated).toContain('swapped from')

await next.patchFile(filePath, origContent)

await check(() => browser.elementByCss('p').text(), /hello from/)
expect(
await renderViaHTTP(next.url, '/client-component-route')
).toContain('hello from')
} finally {
await next.patchFile(filePath, origContent)
}
})
}
})

describe('css support', () => {
Expand Down

0 comments on commit e816e45

Please sign in to comment.