Filter proper chunks from chunk group for client components (#38379)

Client components might result in a page chunk or a standalone splitted chunk marked in flight manifest, this PR filters out the page chunk for standalone chunk so that while loading a client chunk (like for `next/link`) it won't load the page chunk to break the hydration. `chunk.ids` is not enough for getting required chunks, so we get the chunks from chunk group then filter the required ones. tests: Re-enable few previous rsc tests chore: refactor few webpack api usage ## Bug - [ ] Related issues linked using `fixes #number` - [x] Integration tests added - [ ] Errors have helpful link attached, see `contributing.md`
2022-07-06 19:35:20 +02:00 · 2022-07-06 19:35:20 +02:00 · a16d8dd4cd
commit a16d8dd4cd
parent f2b8a6b60f
5 changed files with 139 additions and 137 deletions
--- a/packages/next/build/webpack/plugins/flight-manifest-plugin.ts
+++ b/packages/next/build/webpack/plugins/flight-manifest-plugin.ts
@ -10,6 +10,7 @@ import { FLIGHT_MANIFEST } from '../../../shared/lib/constants'
 import { clientComponentRegex } from '../loaders/utils'
 import { relative } from 'path'
 import { getEntrypointFiles } from './build-manifest-plugin'
+import type { webpack5 } from 'next/dist/compiled/webpack/webpack'

 // This is the module that will be used to anchor all client references to.
 // I.e. it will have all the client files as async deps from this point on.
@ -66,13 +67,17 @@ export class FlightManifestPlugin {
    })
  }

-  createAsset(assets: any, compilation: any, context: string) {
+  createAsset(assets: any, compilation: webpack5.Compilation, context: string) {
    const manifest: any = {}
    const appDir = this.appDir
    const dev = this.dev

    compilation.chunkGroups.forEach((chunkGroup: any) => {
-      function recordModule(chunk: any, id: string | number, mod: any) {
+      function recordModule(
+        chunk: webpack5.Chunk,
+        id: string | number,
+        mod: any
+      ) {
        const resource: string = mod.resource

        // TODO: Hook into deps instead of the target module.
@ -120,46 +125,49 @@ export class FlightManifestPlugin {

        const moduleExportedKeys = ['', '*']
          .concat(
-            [...exportsInfo.exports].map((exportInfo) => {
-              if (exportInfo.provided) {
-                return exportInfo.name
-              }
-              return null
-            }),
+            [...exportsInfo.exports]
+              .filter((exportInfo) => exportInfo.provided)
+              .map((exportInfo) => exportInfo.name),
            ...cjsExports
          )
          .filter((name) => name !== null)

        // Get all CSS files imported in that chunk.
        const cssChunks: string[] = []
-        for (const entrypoint of chunk._groups) {
-          if (entrypoint.getFiles) {
-            const files = getEntrypointFiles(entrypoint)
-            for (const file of files) {
-              if (file.endsWith('.css')) {
-                cssChunks.push(file)
-              }
+        for (const entrypoint of chunk.groupsIterable) {
+          const files = getEntrypointFiles(entrypoint)
+          for (const file of files) {
+            if (file.endsWith('.css')) {
+              cssChunks.push(file)
            }
          }
        }

        moduleExportedKeys.forEach((name) => {
+          let requiredChunks = []
          if (!moduleExports[name]) {
+            const isRelatedChunk = (c: webpack5.Chunk) =>
+              // If current chunk is a page, it should require the related page chunk;
+              // If current chunk is a component, it should filter out the related page chunk;
+              chunk.name?.startsWith('pages/') || !c.name?.startsWith('pages/')
+
+            if (appDir) {
+              requiredChunks = chunkGroup.chunks
+                .filter(isRelatedChunk)
+                .map((requiredChunk: webpack5.Chunk) => {
+                  return (
+                    requiredChunk.id +
+                    ':' +
+                    (requiredChunk.name || requiredChunk.id) +
+                    (dev ? '' : '-' + requiredChunk.hash)
+                  )
+                })
+            }
+
            moduleExports[name] = {
              id,
              name,
-              chunks: appDir
-                ? chunk.ids
-                    .map((chunkId: string) => {
-                      return (
-                        chunkId +
-                        ':' +
-                        (chunk.name || chunkId) +
-                        (dev ? '' : '-' + chunk.hash)
-                      )
-                    })
-                    .concat(cssChunks)
-                : [],
+              chunks: requiredChunks.concat(cssChunks),
            }
          }
          if (!moduleIdMapping[id][name]) {
@ -174,7 +182,7 @@ export class FlightManifestPlugin {
        manifest.__ssr_module_mapping__ = moduleIdMapping
      }

-      chunkGroup.chunks.forEach((chunk: any) => {
+      chunkGroup.chunks.forEach((chunk: webpack5.Chunk) => {
        const chunkModules =
          compilation.chunkGraph.getChunkModulesIterable(chunk)
        for (const mod of chunkModules) {
@ -183,8 +191,9 @@ export class FlightManifestPlugin {
          recordModule(chunk, modId, mod)

          // If this is a concatenation, register each child to the parent ID.
-          if (mod.modules) {
-            mod.modules.forEach((concatenatedMod: any) => {
+          const anyModule = mod as any
+          if (anyModule.modules) {
+            anyModule.modules.forEach((concatenatedMod: any) => {
              recordModule(chunk, modId, concatenatedMod)
            })
          }
--- a/test/integration/react-server-components/basic/app/page.server.js
+++ b/test/integration/react-server-components/basic/app/page.server.js
@ -1,6 +1,4 @@
 import Nav from '../components/nav'
-import Script from 'next/script'
-import Head from 'next/head'

 const envVar = process.env.ENV_VAR_TEST
 const headerKey = 'x-next-test-client'
@ -8,15 +6,10 @@ const headerKey = 'x-next-test-client'
 export default function Index({ header }) {
  return (
    <div>
-      <Head>
-        <meta name="rsc-title" content="index" />
-        <title>{`hello, ${envVar}`}</title>
-      </Head>
      <h1>{`component:index.server`}</h1>
      <div>{'env:' + envVar}</div>
      <div>{'header:' + header}</div>
      <Nav />
-      <Script id="client-script">{`;`}</Script>
    </div>
  )
 }
--- a/test/integration/react-server-components/test/index.test.js
+++ b/test/integration/react-server-components/test/index.test.js
@ -2,11 +2,14 @@
 /* eslint-disable jest/no-commented-out-tests */
 import { join } from 'path'
 import fs from 'fs-extra'
-import { File, runDevSuite, runProdSuite } from 'next-test-utils'
+import { runDevSuite, runProdSuite } from 'next-test-utils'
 import rsc from './rsc'

 const appDir = join(__dirname, '../basic')
-const nextConfig = new File(join(appDir, 'next.config.js'))
+const nodeArgs = [
+  '-r',
+  join(appDir, '../../../lib/react-channel-require-hook.js'),
+]

 /* TODO: support edge runtime in the future
 const edgeRuntimeBasicSuite = {
@ -96,14 +99,14 @@ const nodejsRuntimeBasicSuite = {
      })
    }
  },
-  beforeAll: () => {},
-  afterAll: () => {
-    nextConfig.restore()
-  },
+}
+
+const options = {
+  nodeArgs,
  env: {
    __NEXT_REACT_CHANNEL: 'exp',
  },
 }

-runDevSuite('Node.js runtime', appDir, nodejsRuntimeBasicSuite)
-runProdSuite('Node.js runtime', appDir, nodejsRuntimeBasicSuite)
+runDevSuite('Node.js runtime', appDir, nodejsRuntimeBasicSuite, options)
+runProdSuite('Node.js runtime', appDir, nodejsRuntimeBasicSuite, options)
--- a/test/integration/react-server-components/test/rsc.js
+++ b/test/integration/react-server-components/test/rsc.js
@ -34,24 +34,21 @@ export default function (context, { runtime, env }) {
  })

  // TODO: support RSC index route
-  it.skip('should render server components correctly', async () => {
+  it('should render server components correctly', async () => {
    const homeHTML = await renderViaHTTP(context.appPort, '/', null, {
      headers: {
        'x-next-test-client': 'test-util',
      },
    })

-    const browser = await webdriver(context.appPort, '/')
-    const scriptTagContent = await browser.elementById('client-script').text()
    // should have only 1 DOCTYPE
    expect(homeHTML).toMatch(/^<!DOCTYPE html><html/)
-    expect(homeHTML).toMatch('<meta name="rsc-title" content="index"/>')
+    // TODO: support next/head
+    // expect(homeHTML).toMatch('<meta name="rsc-title" content="index"/>')
    expect(homeHTML).toContain('component:index.server')
-    expect(homeHTML).toContain('env:env_var_test')
+    // TODO: support env
+    // expect(homeHTML).toContain('env:env_var_test')
    expect(homeHTML).toContain('header:test-util')
-    expect(homeHTML).toMatch(/<\/body><\/html>$/)
-
-    expect(scriptTagContent).toBe(';')

    const inlineFlightContents = []
    const $ = cheerio.load(homeHTML)
@ -150,104 +147,101 @@ export default function (context, { runtime, env }) {
    expect(dynamicRoute1HTML).toContain('router pathname: /routes/[dynamic]')
  })

-  // FIXME: chunks missing in prod mode
-  if (env === 'dev') {
-    it('should be able to navigate between rsc pages', async () => {
-      const browser = await webdriver(context.appPort, '/root')
+  it('should be able to navigate between rsc pages', async () => {
+    const browser = await webdriver(context.appPort, '/root')

-      await browser.waitForElementByCss('#goto-next-link').click()
-      await new Promise((res) => setTimeout(res, 1000))
-      expect(await browser.url()).toBe(
-        `http://localhost:${context.appPort}/next-api/link`
-      )
-      await browser.waitForElementByCss('#goto-home').click()
-      await new Promise((res) => setTimeout(res, 1000))
-      expect(await browser.url()).toBe(
-        `http://localhost:${context.appPort}/root`
-      )
-      const content = await browser.elementByCss('body').text()
-      expect(content).toContain('component:root.server')
+    await browser.waitForElementByCss('#goto-next-link').click()
+    await new Promise((res) => setTimeout(res, 1000))
+    expect(await browser.url()).toBe(
+      `http://localhost:${context.appPort}/next-api/link`
+    )
+    await browser.waitForElementByCss('#goto-home').click()
+    await new Promise((res) => setTimeout(res, 1000))
+    expect(await browser.url()).toBe(`http://localhost:${context.appPort}/root`)
+    const content = await browser.elementByCss('body').text()
+    expect(content).toContain('component:root.server')

-      await browser.waitForElementByCss('#goto-streaming-rsc').click()
+    await browser.waitForElementByCss('#goto-streaming-rsc').click()

-      // Wait for navigation and streaming to finish.
-      await check(
-        () => browser.elementByCss('#content').text(),
-        'next_streaming_data'
-      )
-      expect(await browser.url()).toBe(
-        `http://localhost:${context.appPort}/streaming-rsc`
-      )
-    })
+    // Wait for navigation and streaming to finish.
+    await check(
+      () => browser.elementByCss('#content').text(),
+      'next_streaming_data'
+    )
+    expect(await browser.url()).toBe(
+      `http://localhost:${context.appPort}/streaming-rsc`
+    )
+  })

-    it('should handle streaming server components correctly', async () => {
-      const browser = await webdriver(context.appPort, '/streaming-rsc')
-      const content = await browser.eval(
-        `document.querySelector('#content').innerText`
-      )
-      expect(content).toMatchInlineSnapshot('"next_streaming_data"')
-    })
+  it('should handle streaming server components correctly', async () => {
+    const browser = await webdriver(context.appPort, '/streaming-rsc')
+    const content = await browser.eval(
+      `document.querySelector('#content').innerText`
+    )
+    expect(content).toMatchInlineSnapshot('"next_streaming_data"')
+  })

-    it('should support next/link in server components', async () => {
-      const linkHTML = await renderViaHTTP(context.appPort, '/next-api/link')
-      const linkText = getNodeBySelector(
-        linkHTML,
-        'body > div > a[href="/root"]'
-      ).text()
+  it('should support next/link in server components', async () => {
+    const linkHTML = await renderViaHTTP(context.appPort, '/next-api/link')
+    const linkText = getNodeBySelector(
+      linkHTML,
+      'body > div > a[href="/root"]'
+    ).text()

-      expect(linkText).toContain('home')
+    expect(linkText).toContain('home')

-      const browser = await webdriver(context.appPort, '/next-api/link')
+    const browser = await webdriver(context.appPort, '/next-api/link')

-      // We need to make sure the app is fully hydrated before clicking, otherwise
-      // it will be a full redirection instead of being taken over by the next
-      // router. This timeout prevents it being flaky caused by fast refresh's
-      // rebuilding event.
-      await new Promise((res) => setTimeout(res, 1000))
-      await browser.eval('window.beforeNav = 1')
+    // We need to make sure the app is fully hydrated before clicking, otherwise
+    // it will be a full redirection instead of being taken over by the next
+    // router. This timeout prevents it being flaky caused by fast refresh's
+    // rebuilding event.
+    await new Promise((res) => setTimeout(res, 1000))
+    await browser.eval('window.beforeNav = 1')

-      await browser.waitForElementByCss('#next_id').click()
-      await check(() => browser.elementByCss('#query').text(), 'query:1')
+    await browser.waitForElementByCss('#next_id').click()
+    await check(() => browser.elementByCss('#query').text(), 'query:1')

-      await browser.waitForElementByCss('#next_id').click()
-      await check(() => browser.elementByCss('#query').text(), 'query:2')
+    await browser.waitForElementByCss('#next_id').click()
+    await check(() => browser.elementByCss('#query').text(), 'query:2')

+    if (env === 'dev') {
      expect(await browser.eval('window.beforeNav')).toBe(1)
+    }
+  })
+
+  it('should refresh correctly with next/link', async () => {
+    // Select the button which is not hidden but rendered
+    const selector = '#goto-next-link'
+    let hasFlightRequest = false
+    const browser = await webdriver(context.appPort, '/root', {
+      beforePageLoad(page) {
+        page.on('request', (request) => {
+          const url = request.url()
+          if (/\?__flight__=1/.test(url)) {
+            hasFlightRequest = true
+          }
+        })
+      },
    })

-    it('should refresh correctly with next/link', async () => {
-      // Select the button which is not hidden but rendered
-      const selector = '#goto-next-link'
-      let hasFlightRequest = false
-      const browser = await webdriver(context.appPort, '/root', {
-        beforePageLoad(page) {
-          page.on('request', (request) => {
-            const url = request.url()
-            if (/\?__flight__=1/.test(url)) {
-              hasFlightRequest = true
-            }
-          })
-        },
-      })
+    // wait for hydration
+    await new Promise((res) => setTimeout(res, 1000))
+    if (env === 'dev') {
+      expect(hasFlightRequest).toBe(false)
+    }
+    await browser.elementByCss(selector).click()

-      // wait for hydration
-      await new Promise((res) => setTimeout(res, 1000))
-      if (env === 'dev') {
-        expect(hasFlightRequest).toBe(false)
-      }
-      await browser.elementByCss(selector).click()
-
-      // wait for re-hydration
-      if (env === 'dev') {
-        await check(
-          () => (hasFlightRequest ? 'success' : hasFlightRequest),
-          'success'
-        )
-      }
-      const refreshText = await browser.elementByCss(selector).text()
-      expect(refreshText).toBe('next link')
-    })
-  }
+    // wait for re-hydration
+    if (env === 'dev') {
+      await check(
+        () => (hasFlightRequest ? 'success' : hasFlightRequest),
+        'success'
+      )
+    }
+    const refreshText = await browser.elementByCss(selector).text()
+    expect(refreshText).toBe('next link')
+  })

  it('should escape streaming data correctly', async () => {
    const browser = await webdriver(context.appPort, '/escaping-rsc')
@ -347,7 +341,7 @@ export default function (context, { runtime, env }) {
    expect(getNodeBySelector(pageUnknownHTML, id).text()).toBe(content)
  })

-  it.skip('should support streaming for flight response', async () => {
+  it('should support streaming for flight response', async () => {
    await fetchViaHTTP(context.appPort, '/?__flight__=1').then(
      async (response) => {
        const result = await resolveStreamResponse(response)
--- a/test/lib/next-test-utils.js
+++ b/test/lib/next-test-utils.js
@ -768,6 +768,7 @@ function runSuite(suiteName, context, options) {
          stderr: true,
          stdout: true,
          env: options.env || {},
+          nodeArgs: options.nodeArgs,
        })
        context.stdout = stdout
        context.stderr = stderr
@ -776,6 +777,7 @@ function runSuite(suiteName, context, options) {
          onStderr,
          onStdout,
          env: options.env || {},
+          nodeArgs: options.nodeArgs,
        })
      } else if (env === 'dev') {
        context.appPort = await findPort()
@ -783,6 +785,7 @@ function runSuite(suiteName, context, options) {
          onStderr,
          onStdout,
          env: options.env || {},
+          nodeArgs: options.nodeArgs,
        })
      }
    })