Skip to content

Commit

Permalink
Should resolve esm external module imports on server (#40865)
Browse files Browse the repository at this point in the history
### Issue

When import an esm package in client component, and use it in server
component page, it will fail to SSR but render successfully on client.
It's because the import to esm package will make the client chunk become
an **async module** since esm module will be treated as **async**.

```
page (serve component) -> local module (client) -> external dependency (esm)
```

Then in react SSR layer, it need the module type information of that
chunk, async or not for react so that react could unwrap the async
module from `Promise` properly when SSR.

### Solution

We need to mark the client entries which are effected by async/esm
modules that becoming **async** as `async: true` in SSR manifest.

Since flight manifest plugin is only running against client compiler,
which doesn't have those module information from server compiler. So we
collect the async modules from the **server** compiler **client** layer
from flight entry client plugin, then leverage the collection to detect
if a module is async in flight manifest plugin for react.

## Bug

- [ ] Related issues linked using `fixes #number`
- [x] Integration tests added
- [ ] Errors have a helpful link attached, see `contributing.md`
  • Loading branch information
huozhi committed Sep 26, 2022
1 parent 82682a3 commit 2af6b63
Show file tree
Hide file tree
Showing 8 changed files with 87 additions and 54 deletions.
11 changes: 2 additions & 9 deletions packages/next/build/webpack-config.ts
Expand Up @@ -1007,14 +1007,7 @@ export default async function getBaseWebpackConfig(
alias: process.env.__NEXT_REACT_CHANNEL
? {
react: `react-${process.env.__NEXT_REACT_CHANNEL}`,
'react/package.json': `react-${process.env.__NEXT_REACT_CHANNEL}/package.json`,
'react/jsx-runtime': `react-${process.env.__NEXT_REACT_CHANNEL}/jsx-runtime`,
'react/jsx-dev-runtime': `react-${process.env.__NEXT_REACT_CHANNEL}/jsx-dev-runtime`,
'react-dom': `react-dom-${process.env.__NEXT_REACT_CHANNEL}`,
'react-dom/package.json': `react-dom-${process.env.__NEXT_REACT_CHANNEL}/package.json`,
'react-dom/server': `react-dom-${process.env.__NEXT_REACT_CHANNEL}/server`,
'react-dom/server.browser': `react-dom-${process.env.__NEXT_REACT_CHANNEL}/server.browser`,
'react-dom/client': `react-dom-${process.env.__NEXT_REACT_CHANNEL}/client`,
}
: false,
conditionNames: ['react-server'],
Expand Down Expand Up @@ -1172,10 +1165,10 @@ export default async function getBaseWebpackConfig(
context,
request
)
return resolved
return `${externalType} ${resolved}`
} catch (err) {
// The `react-server` condition is not matched, fallback.
return
// The `react-server` condition is not matched, fallback.
}
}

Expand Down
Expand Up @@ -16,7 +16,8 @@ import {
COMPILER_NAMES,
FLIGHT_SERVER_CSS_MANIFEST,
} from '../../../shared/lib/constants'
import { FlightCSSManifest } from './flight-manifest-plugin'
import { FlightCSSManifest, traverseModules } from './flight-manifest-plugin'
import { ASYNC_CLIENT_MODULES } from './flight-manifest-plugin'
import { isClientComponentModule } from '../loaders/utils'

interface Options {
Expand Down Expand Up @@ -61,6 +62,18 @@ export class FlightClientEntryPlugin {
compiler.hooks.finishMake.tapPromise(PLUGIN_NAME, (compilation) => {
return this.createClientEntries(compiler, compilation)
})

compiler.hooks.afterCompile.tap(PLUGIN_NAME, (compilation) => {
traverseModules(compilation, (mod) => {
// The module must has request, and resource so it's not a new entry created with loader.
// Using the client layer module, which doesn't have `rsc` tag in buildInfo.
if (mod.request && mod.resource && !mod.buildInfo.rsc) {
if (compilation.moduleGraph.isAsync(mod)) {
ASYNC_CLIENT_MODULES.add(mod.resource)
}
}
})
})
}

async createClientEntries(compiler: any, compilation: any) {
Expand Down Expand Up @@ -372,6 +385,7 @@ export class FlightClientEntryPlugin {
compilation.hooks.failedEntry.call(entry, options, err)
return reject(err)
}

compilation.hooks.succeedEntry.call(entry, options, module)
return resolve(module)
}
Expand Down
60 changes: 45 additions & 15 deletions packages/next/build/webpack/plugins/flight-manifest-plugin.ts
Expand Up @@ -43,6 +43,11 @@ interface ManifestNode {
* Chunks for the module. JS and CSS.
*/
chunks: ManifestChunks

/**
* If chunk contains async module
*/
async?: boolean
}
}

Expand All @@ -60,6 +65,37 @@ export type FlightCSSManifest = {

const PLUGIN_NAME = 'FlightManifestPlugin'

// Collect modules from server/edge compiler in client layer,
// and detect if it's been used, and mark it as `async: true` for react.
// So that react could unwrap the async module from promise and render module itself.
export const ASYNC_CLIENT_MODULES = new Set<string>()

export function traverseModules(
compilation: webpack.Compilation,
callback: (
mod: any,
chunk: webpack.Chunk,
chunkGroup: typeof compilation.chunkGroups[0]
) => any
) {
compilation.chunkGroups.forEach((chunkGroup) => {
chunkGroup.chunks.forEach((chunk: webpack.Chunk) => {
const chunkModules = compilation.chunkGraph.getChunkModulesIterable(
chunk
// TODO: Update type so that it doesn't have to be cast.
) as Iterable<webpack.NormalModule>
for (const mod of chunkModules) {
callback(mod, chunk, chunkGroup)
const anyModule = mod as any
if (anyModule.modules) {
for (const subMod of anyModule.modules)
callback(subMod, chunk, chunkGroup)
}
}
})
})
}

export class FlightManifestPlugin {
dev: Options['dev'] = false

Expand Down Expand Up @@ -117,21 +153,7 @@ export class FlightManifestPlugin {
}
}

compilation.chunkGroups.forEach((chunkGroup) => {
chunkGroup.chunks.forEach((chunk: webpack.Chunk) => {
const chunkModules = compilation.chunkGraph.getChunkModulesIterable(
chunk
// TODO: Update type so that it doesn't have to be cast.
) as Iterable<webpack.NormalModule>
for (const mod of chunkModules) {
collectClientRequest(mod)
const anyModule = mod as any
if (anyModule.modules) {
for (const subMod of anyModule.modules) collectClientRequest(subMod)
}
}
})
})
traverseModules(compilation, (mod) => collectClientRequest(mod))

compilation.chunkGroups.forEach((chunkGroup) => {
const cssResourcesInChunkGroup = new Set<string>()
Expand Down Expand Up @@ -208,6 +230,8 @@ export class FlightManifestPlugin {
}

const exportsInfo = compilation.moduleGraph.getExportsInfo(mod)
const isAsyncModule = ASYNC_CLIENT_MODULES.has(mod.resource)

const cjsExports = [
...new Set([
...mod.dependencies.map((dep) => {
Expand Down Expand Up @@ -260,6 +284,10 @@ export class FlightManifestPlugin {
id,
name,
chunks: requiredChunks,
// E.g.
// page (server) -> local module (client) -> package (esm)
// The esm package will bubble up to make the entire chain till the client entry as async module.
async: isAsyncModule,
}
}

Expand Down Expand Up @@ -305,6 +333,8 @@ export class FlightManifestPlugin {
const file = 'server/' + FLIGHT_MANIFEST
const json = JSON.stringify(manifest)

ASYNC_CLIENT_MODULES.clear()

assets[file + '.js'] = new sources.RawSource(
'self.__RSC_MANIFEST=' + json
// Work around webpack 4 type of RawSource being used
Expand Down
43 changes: 18 additions & 25 deletions test/e2e/app-dir/rsc-basic.test.ts
Expand Up @@ -74,12 +74,10 @@ describe('app dir - react server components', () => {

// should have only 1 DOCTYPE
expect(homeHTML).toMatch(/^<!DOCTYPE html><html/)
// TODO: support next/head
// expect(homeHTML).toMatch('<meta name="rsc-title" content="index"/>')
expect(homeHTML).toContain('component:index.server')
// TODO: support env
// expect(homeHTML).toContain('env:env_var_test')
expect(homeHTML).toContain('header:test-util')
// support esm module on server side
expect(homeHTML).toContain('random-module-instance')

const inlineFlightContents = []
const $ = cheerio.load(homeHTML)
Expand Down Expand Up @@ -134,26 +132,6 @@ describe('app dir - react server components', () => {
expect(html).toContain('foo.client')
})

it('should resolve different kinds of components correctly', async () => {
const html = await renderViaHTTP(next.url, '/shared')
const main = getNodeBySelector(html, '#main').html()

// Should have 5 occurrences of "client_component".
expect(Array.from(main.matchAll(/client_component/g)).length).toBe(5)

// Should have 2 occurrences of "shared:server", and 2 occurrences of
// "shared:client".
const sharedServerModule = Array.from(main.matchAll(/shared:server:(\d+)/g))
const sharedClientModule = Array.from(main.matchAll(/shared:client:(\d+)/g))
expect(sharedServerModule.length).toBe(2)
expect(sharedClientModule.length).toBe(2)

// Should have 2 modules created for the shared component.
expect(sharedServerModule[0][1]).toBe(sharedServerModule[1][1])
expect(sharedClientModule[0][1]).toBe(sharedClientModule[1][1])
expect(sharedServerModule[0][1]).not.toBe(sharedClientModule[0][1])
})

it('should be able to navigate between rsc routes', async () => {
const browser = await webdriver(next.url, '/root')

Expand Down Expand Up @@ -304,10 +282,25 @@ describe('app dir - react server components', () => {
expect(content).toContain('foo.client')
})

it('should support the re-export syntax in server component', async () => {
it('should resolve different kinds of components correctly', async () => {
const html = await renderViaHTTP(next.url, '/shared')
const main = getNodeBySelector(html, '#main').html()
const content = getNodeBySelector(html, '#bar').text()

// Should have 5 occurrences of "client_component".
expect(Array.from(main.matchAll(/client_component/g)).length).toBe(5)

// Should have 2 occurrences of "shared:server", and 2 occurrences of
// "shared:client".
const sharedServerModule = Array.from(main.matchAll(/shared:server:(\d+)/g))
const sharedClientModule = Array.from(main.matchAll(/shared:client:(\d+)/g))
expect(sharedServerModule.length).toBe(2)
expect(sharedClientModule.length).toBe(2)

// Should have 2 modules created for the shared component.
expect(sharedServerModule[0][1]).toBe(sharedServerModule[1][1])
expect(sharedClientModule[0][1]).toBe(sharedClientModule[1][1])
expect(sharedServerModule[0][1]).not.toBe(sharedClientModule[0][1])
expect(content).toContain('bar.server.js:')
})

Expand Down
4 changes: 3 additions & 1 deletion test/e2e/app-dir/rsc-basic/app/page.js
@@ -1,10 +1,11 @@
import Nav from '../components/nav'
import { headers } from 'next/dist/client/components/hooks-server'
import { name } from 'random-module-instance'

const envVar = process.env.ENV_VAR_TEST
const headerKey = 'x-next-test-client'

export default function Index(props) {
export default function Index() {
const headersList = headers()
const header = headersList.get(headerKey)

Expand All @@ -13,6 +14,7 @@ export default function Index(props) {
<h1>{`component:index.server`}</h1>
<div>{'env:' + envVar}</div>
<div>{'header:' + header}</div>
<p>{name}</p>
<Nav />
</div>
)
Expand Down
1 change: 0 additions & 1 deletion test/e2e/app-dir/rsc-basic/app/shared/page.js
Expand Up @@ -14,7 +14,6 @@ export default function Page() {
<div id="main" suppressHydrationWarning>
<Random />
<br />
<br />
<ClientFromDirect />
<br />
<ClientFromShared />
Expand Down
@@ -1 +1,2 @@
exports.random = ~~(Math.random() * 1e5)
export const random = ~~(Math.random() * 1e5)
export const name = 'random-module-instance'
@@ -1,4 +1,5 @@
{
"name": "random-module-instance",
"main": "./index.js"
"type": "module",
"exports": "./index.js"
}

0 comments on commit 2af6b63

Please sign in to comment.