From a1e76fb96678b0ac3d7a74c28d2d005516ef1cc0 Mon Sep 17 00:00:00 2001 From: Spencer Elliott Date: Tue, 9 Nov 2021 08:42:23 -0500 Subject: [PATCH] Use require.resolve to detect "framework" packages (fix pnpm) (#21048) Co-authored-by: Tim Neutkens --- packages/next/build/webpack-config.ts | 44 ++++- .../pnpm-support/app-multi-page/.babelrc | 3 + .../pnpm-support/app-multi-page/package.json | 14 ++ .../app-multi-page/pages/about.js | 17 ++ .../app-multi-page/pages/index.js | 12 ++ test/integration/pnpm-support/app/.babelrc | 3 + .../integration/pnpm-support/app/package.json | 4 +- .../pnpm-support/app/pages/index.js | 4 +- .../pnpm-support/test/index.test.js | 174 +++++++++++------- test/lib/next-test-utils.js | 10 +- 10 files changed, 210 insertions(+), 75 deletions(-) create mode 100644 test/integration/pnpm-support/app-multi-page/.babelrc create mode 100644 test/integration/pnpm-support/app-multi-page/package.json create mode 100644 test/integration/pnpm-support/app-multi-page/pages/about.js create mode 100644 test/integration/pnpm-support/app-multi-page/pages/index.js create mode 100644 test/integration/pnpm-support/app/.babelrc diff --git a/packages/next/build/webpack-config.ts b/packages/next/build/webpack-config.ts index 81e9beb63207510..e01cb621e866705 100644 --- a/packages/next/build/webpack-config.ts +++ b/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: /(? + resource.startsWith(packagePath) + ) + }, priority: 40, // Don't let webpack eliminate this chunk (prevents this chunk from // becoming a part of the commons chunk) diff --git a/test/integration/pnpm-support/app-multi-page/.babelrc b/test/integration/pnpm-support/app-multi-page/.babelrc new file mode 100644 index 000000000000000..1ff94f7ed28e16b --- /dev/null +++ b/test/integration/pnpm-support/app-multi-page/.babelrc @@ -0,0 +1,3 @@ +{ + "presets": ["next/babel"] +} diff --git a/test/integration/pnpm-support/app-multi-page/package.json b/test/integration/pnpm-support/app-multi-page/package.json new file mode 100644 index 000000000000000..ae87c0708d1918d --- /dev/null +++ b/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" + } +} diff --git a/test/integration/pnpm-support/app-multi-page/pages/about.js b/test/integration/pnpm-support/app-multi-page/pages/about.js new file mode 100644 index 000000000000000..8ef178ac21074fa --- /dev/null +++ b/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

Hello {loaded && World}

+} + +export default IndexPage diff --git a/test/integration/pnpm-support/app-multi-page/pages/index.js b/test/integration/pnpm-support/app-multi-page/pages/index.js new file mode 100644 index 000000000000000..378de0f5c4d68e6 --- /dev/null +++ b/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

Hello {loaded && World}

+} + +export default IndexPage diff --git a/test/integration/pnpm-support/app/.babelrc b/test/integration/pnpm-support/app/.babelrc new file mode 100644 index 000000000000000..1ff94f7ed28e16b --- /dev/null +++ b/test/integration/pnpm-support/app/.babelrc @@ -0,0 +1,3 @@ +{ + "presets": ["next/babel"] +} diff --git a/test/integration/pnpm-support/app/package.json b/test/integration/pnpm-support/app/package.json index 3cc6af9e7f671c4..b39d0e9d6bc632a 100644 --- a/test/integration/pnpm-support/app/package.json +++ b/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" } } diff --git a/test/integration/pnpm-support/app/pages/index.js b/test/integration/pnpm-support/app/pages/index.js index 69192c68a0ce537..313efcba7382bfd 100644 --- a/test/integration/pnpm-support/app/pages/index.js +++ b/test/integration/pnpm-support/app/pages/index.js @@ -1 +1,3 @@ -export default () =>

Hello World

+const IndexPage = () =>

Hello World

+ +export default IndexPage diff --git a/test/integration/pnpm-support/test/index.test.js b/test/integration/pnpm-support/test/index.test.js index 8a656bd8f34032f..65e1c30749df5e6 100644 --- a/test/integration/pnpm-support/test/index.test.js +++ b/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) + } }) }) }) diff --git a/test/lib/next-test-utils.js b/test/lib/next-test-utils.js index 0dbe023da0ff68f..99a5b66ce382646 100644 --- a/test/lib/next-test-utils.js +++ b/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()