Use require.resolve to detect "framework" packages (fix pnpm) (#21048)

Co-authored-by: Tim Neutkens <tim@timneutkens.nl>

packages/next/build/webpack-config.ts

@@ -754,6 +754,36 @@ export default async function getBaseWebpackConfig(
     )
   }
 
+  const getPackagePath = (name: string, relativeToPath: string) => {
+    const packageJsonPath = require.resolve(`${name}/package.json`, {
+      paths: [relativeToPath],
+    })
+    // Include a trailing slash so that a `.startsWith(packagePath)` check avoids false positives
+    // when one package name starts with the full name of a different package.
+    // For example:
+    //   "node_modules/react-slider".startsWith("node_modules/react")  // true
+    //   "node_modules/react-slider".startsWith("node_modules/react/") // false
+    return path.join(packageJsonPath, '../')
+  }
+
+  // Packages which will be split into the 'framework' chunk.
+  // Only top-level packages are included, e.g. nested copies like
+  // 'node_modules/meow/node_modules/object-assign' are not included.
+  const topLevelFrameworkPaths = [
+    getPackagePath('react', dir),
+    getPackagePath('react-dom', dir),
+    getPackagePath('scheduler', require.resolve('react-dom', { paths: [dir] })),
+    getPackagePath('object-assign', require.resolve('react', { paths: [dir] })),
+    getPackagePath(
+      'object-assign',
+      require.resolve('react-dom', { paths: [dir] })
+    ),
+    getPackagePath(
+      'use-subscription',
+      require.resolve('next', { paths: [dir] })
+    ),
+  ]
+
   // Select appropriate SplitChunksPlugin config for this build
   const splitChunksConfig: webpack.Options.SplitChunksOptions | false = dev
     ? false
@@ -770,10 +800,16 @@ export default async function getBaseWebpackConfig(
             chunks: (chunk: webpack.compilation.Chunk) =>
               !chunk.name?.match(MIDDLEWARE_ROUTE),
             name: 'framework',
-            // This regex ignores nested copies of framework libraries so they're
-            // bundled with their issuer.
-            // https://github.com/vercel/next.js/pull/9012
-            test: /(?<!node_modules.*)[\\/]node_modules[\\/](react|react-dom|scheduler|prop-types|use-subscription)[\\/]/,
+            test(module) {
+              const resource =
+                module.nameForCondition && module.nameForCondition()
+              if (!resource) {
+                return false
+              }
+              return topLevelFrameworkPaths.some((packagePath) =>
+                resource.startsWith(packagePath)
+              )
+            },
             priority: 40,
             // Don't let webpack eliminate this chunk (prevents this chunk from
             // becoming a part of the commons chunk)

test/integration/pnpm-support/app-multi-page/.babelrc

@@ -0,0 +1,3 @@
+{
+  "presets": ["next/babel"]
+}

test/integration/pnpm-support/app-multi-page/package.json

@@ -0,0 +1,14 @@
+{
+  "name": "pnpm-app-multi-page",
+  "version": "1.0.0",
+  "private": true,
+  "scripts": {
+    "dev": "next dev",
+    "build": "next build",
+    "start": "next start"
+  },
+  "dependencies": {
+    "react": "^17.0.2",
+    "react-dom": "^17.0.2"
+  }
+}

test/integration/pnpm-support/app-multi-page/pages/about.js

@@ -0,0 +1,17 @@
+import React, { useEffect, useState } from 'react'
+
+// Include react-dom to verify that webpack's `splitChunks` works correctly,
+// and client-side JS still executes as expected.
+// See https://github.com/vercel/next.js/issues/20884
+import 'react-dom'
+
+const IndexPage = () => {
+  const [loaded, setLoaded] = useState(false)
+  useEffect(() => {
+    setLoaded(true)
+  }, [])
+
+  return <h1>Hello {loaded && <span id="world">World</span>}</h1>
+}
+
+export default IndexPage

test/integration/pnpm-support/app-multi-page/pages/index.js

@@ -0,0 +1,12 @@
+import React, { useEffect, useState } from 'react'
+
+const IndexPage = () => {
+  const [loaded, setLoaded] = useState(false)
+  useEffect(() => {
+    setLoaded(true)
+  }, [])
+
+  return <h1>Hello {loaded && <span id="world">World</span>}</h1>
+}
+
+export default IndexPage

test/integration/pnpm-support/app/.babelrc

@@ -0,0 +1,3 @@
+{
+  "presets": ["next/babel"]
+}

test/integration/pnpm-support/app/package.json

@@ -8,7 +8,7 @@
     "start": "next start"
   },
   "dependencies": {
-    "react": "^16.7.0",
-    "react-dom": "^16.7.0"
+    "react": "^17.0.2",
+    "react-dom": "^17.0.2"
   }
 }

test/integration/pnpm-support/app/pages/index.js

@@ -1 +1,3 @@
-export default () => <h1>Hello World</h1>
+const IndexPage = () => <h1>Hello World</h1>
+
+export default IndexPage

test/integration/pnpm-support/test/index.test.js

@@ -3,20 +3,19 @@ import execa from 'execa'
 import fs from 'fs-extra'
 import os from 'os'
 import path from 'path'
+import { findPort, killProcess, renderViaHTTP, waitFor } from 'next-test-utils'
+import webdriver from 'next-webdriver'
 
 const packagesDir = path.join(__dirname, '..', '..', '..', '..', 'packages')
-const appDir = path.join(__dirname, '..', 'app')
 
-const runNpm = (cwd, ...args) => execa('npm', [...args], { cwd })
-const runPnpm = async (cwd, ...args) => {
-  try {
-    return await execa('npx', ['pnpm', ...args], { cwd })
-  } catch (err) {
-    console.error({ err })
-    throw err
-  }
+const APP_DIRS = {
+  app: path.join(__dirname, '..', 'app'),
+  'app-multi-page': path.join(__dirname, '..', 'app-multi-page'),
 }
 
+const runNpm = (cwd, ...args) => execa('npm', [...args], { cwd })
+const runPnpm = (cwd, ...args) => execa('npx', ['pnpm', ...args], { cwd })
+
 async function usingTempDir(fn) {
   const folder = path.join(os.tmpdir(), Math.random().toString(36).substring(2))
   await fs.mkdirp(folder)
@@ -47,82 +46,127 @@ async function pack(cwd, pkg) {
 
   if (!tarballFilename) {
     throw new Error(
-      `pnpm failed to pack "next" package tarball in directory ${pkgDir}.`
+      `npm failed to pack "next" package tarball in directory ${pkgDir}.`
     )
   }
 
   return path.join(cwd, tarballFilename)
 }
 
-describe('pnpm support', () => {
-  it('should build with dependencies installed via pnpm', async () => {
-    // Create a Next.js app in a temporary directory, and install dependencies with pnpm.
-    //
-    // "next" and its monorepo dependencies are installed by `npm pack`-ing tarballs from
-    // 'next.js/packages/*', because installing "next" directly via
-    // `pnpm add path/to/next.js/packages/next` results in a symlink:
-    // 'app/node_modules/next' -> 'path/to/next.js/packages/next'.
-    // This is undesired since modules inside "next" would be resolved to the
-    // next.js monorepo 'node_modules' and lead to false test results;
-    // installing from a tarball avoids this issue.
-    //
-    // The "next" package's monorepo dependencies (e.g. "@next/env", "@next/polyfill-module")
-    // are not bundled with `npm pack next.js/packages/next`,
-    // so they need to be packed individually.
-    // To ensure that they are installed upon installing "next", a package.json "pnpm.overrides"
-    // field is used to override these dependency paths at install time.
-    await usingTempDir(async (tempDir) => {
-      console.error('using dir', tempDir)
-      const nextTarballPath = await pack(tempDir, 'next')
-      const dependencyTarballPaths = {
-        '@next/env': await pack(tempDir, 'next-env'),
-        '@next/polyfill-module': await pack(tempDir, 'next-polyfill-module'),
-        '@next/react-dev-overlay': await pack(tempDir, 'react-dev-overlay'),
-        '@next/react-refresh-utils': await pack(tempDir, 'react-refresh-utils'),
-      }
+/**
+ * Create a Next.js app in a temporary directory, and install dependencies with pnpm.
+ *
+ * "next" and its monorepo dependencies are installed by `npm pack`-ing tarballs from
+ * 'next.js/packages/*', because installing "next" directly via
+ * `pnpm add path/to/next.js/packages/next` results in a symlink:
+ * 'app/node_modules/next' -> 'path/to/next.js/packages/next'.
+ * This is undesired since modules inside "next" would be resolved to the
+ * next.js monorepo 'node_modules' and lead to false test results;
+ * installing from a tarball avoids this issue.
+ *
+ * The "next" package's monorepo dependencies (e.g. "@next/env", "@next/polyfill-module")
+ * are not bundled with `npm pack next.js/packages/next`,
+ * so they need to be packed individually.
+ * To ensure that they are installed upon installing "next", a package.json "pnpm.overrides"
+ * field is used to override these dependency paths at install time.
+ */
+async function usingPnpmCreateNextApp(appDir, fn) {
+  await usingTempDir(async (tempDir) => {
+    const nextTarballPath = await pack(tempDir, 'next')
+    const dependencyTarballPaths = {
+      '@next/env': await pack(tempDir, 'next-env'),
+      '@next/polyfill-module': await pack(tempDir, 'next-polyfill-module'),
+      '@next/polyfill-nomodule': await pack(tempDir, 'next-polyfill-nomodule'),
+      '@next/react-dev-overlay': await pack(tempDir, 'react-dev-overlay'),
+      '@next/react-refresh-utils': await pack(tempDir, 'react-refresh-utils'),
+    }
 
-      const tempAppDir = path.join(tempDir, 'app')
-      await fs.copy(appDir, tempAppDir)
-
-      // Inject dependency tarball paths into a "pnpm.overrides" field in package.json,
-      // so that they are installed from packed tarballs rather than from the npm registry.
-      const packageJsonPath = path.join(tempAppDir, 'package.json')
-      const overrides = {}
-      for (const [dependency, tarballPath] of Object.entries(
-        dependencyTarballPaths
-      )) {
-        overrides[dependency] = `file:${tarballPath}`
-      }
-      const packageJsonWithOverrides = {
-        ...(await fs.readJson(packageJsonPath)),
-        pnpm: { overrides },
-      }
-      await fs.writeFile(
-        packageJsonPath,
-        JSON.stringify(packageJsonWithOverrides, null, 2)
-      )
+    const tempAppDir = path.join(tempDir, 'app')
+    await fs.copy(appDir, tempAppDir)
+
+    // Inject dependency tarball paths into a "pnpm.overrides" field in package.json,
+    // so that they are installed from packed tarballs rather than from the npm registry.
+    const packageJsonPath = path.join(tempAppDir, 'package.json')
+    const overrides = {}
+    for (const [dependency, tarballPath] of Object.entries(
+      dependencyTarballPaths
+    )) {
+      overrides[dependency] = `file:${tarballPath}`
+    }
+    const packageJsonWithOverrides = {
+      ...(await fs.readJson(packageJsonPath)),
+      pnpm: { overrides },
+    }
+    await fs.writeFile(
+      packageJsonPath,
+      JSON.stringify(packageJsonWithOverrides, null, 2)
+    )
 
-      await runPnpm(tempAppDir, 'install')
-      await runPnpm(tempAppDir, 'add', `next@${nextTarballPath}`)
+    await runPnpm(tempAppDir, 'install')
+    await runPnpm(tempAppDir, 'add', `next@${nextTarballPath}`)
 
-      await fs.copy(
-        path.join(__dirname, '../../../../packages/next/native'),
-        path.join(tempAppDir, 'node_modules/next/native')
-      )
+    await fs.copy(
+      path.join(__dirname, '../../../../packages/next/native'),
+      path.join(tempAppDir, 'node_modules/next/native')
+    )
+
+    await fn(tempAppDir)
+  })
+}
 
+describe('pnpm support', () => {
+  it('should build with dependencies installed via pnpm', async () => {
+    await usingPnpmCreateNextApp(APP_DIRS['app'], async (appDir) => {
       expect(
-        await fs.pathExists(path.join(tempAppDir, 'pnpm-lock.yaml'))
+        await fs.pathExists(path.join(appDir, 'pnpm-lock.yaml'))
       ).toBeTruthy()
 
+      const packageJsonPath = path.join(appDir, 'package.json')
       const packageJson = await fs.readJson(packageJsonPath)
       expect(packageJson.dependencies['next']).toMatch(/^file:/)
-      for (const dependency of Object.keys(dependencyTarballPaths)) {
+      for (const dependency of [
+        '@next/env',
+        '@next/polyfill-module',
+        '@next/polyfill-nomodule',
+        '@next/react-dev-overlay',
+        '@next/react-refresh-utils',
+      ]) {
         expect(packageJson.pnpm.overrides[dependency]).toMatch(/^file:/)
       }
 
-      const { stdout, stderr } = await runPnpm(tempAppDir, 'next', 'build')
+      const { stdout, stderr } = await runPnpm(appDir, 'run', 'build')
+      console.log(stdout, stderr)
+      expect(stdout).toMatch(/Compiled successfully/)
+    })
+  })
+
+  it('should execute client-side JS on each page', async () => {
+    await usingPnpmCreateNextApp(APP_DIRS['app-multi-page'], async (appDir) => {
+      const { stdout, stderr } = await runPnpm(appDir, 'run', 'build')
       console.log(stdout, stderr)
       expect(stdout).toMatch(/Compiled successfully/)
+
+      let appPort
+      let appProcess
+      let browser
+      try {
+        appPort = await findPort()
+        appProcess = runPnpm(appDir, 'run', 'start', '--', '--port', appPort)
+        await waitFor(5000)
+
+        await renderViaHTTP(appPort, '/')
+
+        browser = await webdriver(appPort, '/', false)
+        expect(await browser.waitForElementByCss('#world').text()).toBe('World')
+        await browser.close()
+
+        browser = await webdriver(appPort, '/about', false)
+        expect(await browser.waitForElementByCss('#world').text()).toBe('World')
+        await browser.close()
+      } finally {
+        await killProcess(appProcess.pid)
+        await waitFor(5000)
+      }
     })
   })
 })

test/lib/next-test-utils.js

@@ -347,10 +347,9 @@ export function buildTS(args = [], cwd, env = {}) {
   })
 }
 
-// Kill a launched app
-export async function killApp(instance) {
+export async function killProcess(pid) {
   await new Promise((resolve, reject) => {
-    treeKill(instance.pid, (err) => {
+    treeKill(pid, (err) => {
       if (err) {
         if (
           process.platform === 'win32' &&
@@ -373,6 +372,11 @@ export async function killApp(instance) {
   })
 }
 
+// Kill a launched app
+export async function killApp(instance) {
+  await killProcess(instance.pid)
+}
+
 export async function startApp(app) {
   await app.prepare()
   const handler = app.getRequestHandler()