-
Notifications
You must be signed in to change notification settings - Fork 96
fix: handle node_modules in standalone's dist dir #3282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ab01d87
8d10761
b1be318
e1d032a
7d85c94
d58ed23
e95da0f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,7 @@ import { | |
| writeFile, | ||
| } from 'node:fs/promises' | ||
| import { createRequire } from 'node:module' | ||
| import { dirname, join, resolve, sep } from 'node:path' | ||
| import { dirname, join, relative, sep } from 'node:path' | ||
| import { join as posixJoin, sep as posixSep } from 'node:path/posix' | ||
|
|
||
| import { trace } from '@opentelemetry/api' | ||
|
|
@@ -116,36 +116,53 @@ export const copyNextServerCode = async (ctx: PluginContext): Promise<void> => { | |
| }, | ||
| ) | ||
|
|
||
| await Promise.all( | ||
| paths.map(async (path: string) => { | ||
| const srcPath = join(srcDir, path) | ||
| const destPath = join(destDir, path) | ||
|
|
||
| // If this is the middleware manifest file, replace it with an empty | ||
| // manifest to avoid running middleware again in the server handler. | ||
| if (path === 'server/middleware-manifest.json') { | ||
| try { | ||
| await replaceMiddlewareManifest(srcPath, destPath) | ||
| } catch (error) { | ||
| throw new Error('Could not patch middleware manifest file', { cause: error }) | ||
| } | ||
| const promises = paths.map(async (path: string) => { | ||
| const srcPath = join(srcDir, path) | ||
| const destPath = join(destDir, path) | ||
|
|
||
| return | ||
| // If this is the middleware manifest file, replace it with an empty | ||
| // manifest to avoid running middleware again in the server handler. | ||
| if (path === 'server/middleware-manifest.json') { | ||
| try { | ||
| await replaceMiddlewareManifest(srcPath, destPath) | ||
| } catch (error) { | ||
| throw new Error('Could not patch middleware manifest file', { cause: error }) | ||
| } | ||
|
|
||
| if (path === 'server/functions-config-manifest.json') { | ||
| try { | ||
| await replaceFunctionsConfigManifest(srcPath, destPath) | ||
| } catch (error) { | ||
| throw new Error('Could not patch functions config manifest file', { cause: error }) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| return | ||
| if (path === 'server/functions-config-manifest.json') { | ||
| try { | ||
| await replaceFunctionsConfigManifest(srcPath, destPath) | ||
| } catch (error) { | ||
| throw new Error('Could not patch functions config manifest file', { cause: error }) | ||
| } | ||
|
|
||
| await cp(srcPath, destPath, { recursive: true, force: true }) | ||
| }), | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| await cp(srcPath, destPath, { recursive: true, force: true }) | ||
| }) | ||
|
|
||
| // this is different node_modules than ones handled by `copyNextDependencies` | ||
| // this is under the standalone/.next folder (not standalone/node_modules or standalone/<some-workspace/node_modules) | ||
| // and started to be created by Next.js in some cases in next@16.1.0-canary.3 | ||
| // this node_modules is artificially created and doesn't have equivalent in the repo | ||
| // so we only copy it, without additional symlinks handling | ||
| if (existsSync(join(srcDir, 'node_modules'))) { | ||
| const filter = ctx.constants.IS_LOCAL ? undefined : nodeModulesFilter | ||
| const src = join(srcDir, 'node_modules') | ||
| const dest = join(destDir, 'node_modules') | ||
| await cp(src, dest, { | ||
| recursive: true, | ||
| verbatimSymlinks: true, | ||
| force: true, | ||
| filter, | ||
| }) | ||
| } | ||
|
|
||
| await Promise.all(promises) | ||
| }) | ||
| } | ||
|
|
||
|
|
@@ -290,42 +307,41 @@ async function patchNextModules( | |
|
|
||
| export const copyNextDependencies = async (ctx: PluginContext): Promise<void> => { | ||
| await tracer.withActiveSpan('copyNextDependencies', async () => { | ||
| const entries = await readdir(ctx.standaloneDir) | ||
| const filter = ctx.constants.IS_LOCAL ? undefined : nodeModulesFilter | ||
| const promises: Promise<void>[] = [] | ||
|
|
||
| const nodeModulesLocationsInStandalone = new Set<string>() | ||
| const commonFilter = ctx.constants.IS_LOCAL ? undefined : nodeModulesFilter | ||
|
|
||
| const dotNextDir = join(ctx.standaloneDir, ctx.nextDistDir) | ||
|
|
||
| await cp(ctx.standaloneRootDir, ctx.serverHandlerRootDir, { | ||
| recursive: true, | ||
| verbatimSymlinks: true, | ||
| force: true, | ||
| filter: async (sourcePath: string) => { | ||
| if (sourcePath === dotNextDir) { | ||
| // copy all except the distDir (.next) folder as this is handled in a separate function | ||
| // this will include the node_modules folder as well | ||
| return false | ||
| } | ||
|
|
||
| const promises: Promise<void>[] = entries.map(async (entry) => { | ||
| // copy all except the distDir (.next) folder as this is handled in a separate function | ||
| // this will include the node_modules folder as well | ||
| if (entry === ctx.nextDistDir) { | ||
| return | ||
| } | ||
| const src = join(ctx.standaloneDir, entry) | ||
| const dest = join(ctx.serverHandlerDir, entry) | ||
| await cp(src, dest, { | ||
| recursive: true, | ||
| verbatimSymlinks: true, | ||
| force: true, | ||
| filter, | ||
| }) | ||
| if (sourcePath.endsWith('node_modules')) { | ||
| // keep track of node_modules as we might need to recreate symlinks | ||
| // we are still copying them | ||
| nodeModulesLocationsInStandalone.add(sourcePath) | ||
| } | ||
|
|
||
| if (entry === 'node_modules') { | ||
| await recreateNodeModuleSymlinks(ctx.resolveFromSiteDir('node_modules'), dest) | ||
| } | ||
| // finally apply common filter if defined | ||
| return commonFilter?.(sourcePath) ?? true | ||
| }, | ||
| }) | ||
|
|
||
| // inside a monorepo there is a root `node_modules` folder that contains all the dependencies | ||
| const rootSrcDir = join(ctx.standaloneRootDir, 'node_modules') | ||
| const rootDestDir = join(ctx.serverHandlerRootDir, 'node_modules') | ||
|
|
||
| // use the node_modules tree from the process.cwd() and not the one from the standalone output | ||
| // as the standalone node_modules are already wrongly assembled by Next.js. | ||
| // see: https://github.com/vercel/next.js/issues/50072 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: might be worth still keeping this reference in a comment somewhere even though we aren't "explicitly" handling this anymore?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll look to re-add this context in follow up ~chore to keep this PR mergeable |
||
| if (existsSync(rootSrcDir) && ctx.standaloneRootDir !== ctx.standaloneDir) { | ||
| promises.push( | ||
| cp(rootSrcDir, rootDestDir, { recursive: true, verbatimSymlinks: true, filter }).then(() => | ||
| recreateNodeModuleSymlinks(resolve('node_modules'), rootDestDir), | ||
| ), | ||
| ) | ||
|
Comment on lines
-293
to
-328
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is for the edge case encountered after I expanded test case (last paragraph + second screenshot). It starts with change of "traversing" from and required adjustments to logic to still handle some special cases (like skipping |
||
| for (const nodeModulesLocationInStandalone of nodeModulesLocationsInStandalone) { | ||
| const relativeToRoot = relative(ctx.standaloneRootDir, nodeModulesLocationInStandalone) | ||
| const locationInProject = join(ctx.outputFileTracingRoot, relativeToRoot) | ||
| const locationInServerHandler = join(ctx.serverHandlerRootDir, relativeToRoot) | ||
|
|
||
| promises.push(recreateNodeModuleSymlinks(locationInProject, locationInServerHandler)) | ||
| } | ||
|
|
||
| await Promise.all(promises) | ||
|
|
@@ -451,7 +467,7 @@ export const verifyHandlerDirStructure = async (ctx: PluginContext) => { | |
| // https://github.com/pnpm/pnpm/issues/9654 | ||
| // https://github.com/pnpm/pnpm/issues/5928 | ||
| // https://github.com/pnpm/pnpm/issues/7362 (persisting even though ticket is closed) | ||
| const nodeModulesFilter = async (sourcePath: string) => { | ||
| const nodeModulesFilter = (sourcePath: string) => { | ||
| // Filtering rule for the following packages: | ||
| // - @rspack+binding-linux-x64-musl | ||
| // - @swc+core-linux-x64-musl | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,6 +107,15 @@ test.describe('[PNPM] Package manager', () => { | |
| const date3 = await page.getByTestId('date-now').textContent() | ||
| expect(date3).not.toBe(date2) | ||
| }) | ||
|
|
||
| test('transitive external dependencies are supported', async ({ page, turborepo }) => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🥇 Awesome tests |
||
| const pageResponse = await page.goto(new URL('/transitive-external-deps', turborepo.url).href) | ||
|
|
||
| expect(pageResponse?.status()).toBe(200) | ||
|
|
||
| await expect(page.getByTestId('dep-a-version')).toHaveText('3.10.1') | ||
| await expect(page.getByTestId('dep-b-version')).toHaveText('4.17.21') | ||
| }) | ||
| }) | ||
|
|
||
| test.describe('[NPM] Package manager', () => { | ||
|
|
@@ -228,4 +237,15 @@ test.describe('[NPM] Package manager', () => { | |
| '.env.production.local': 'defined in .env.production.local', | ||
| }) | ||
| }) | ||
|
|
||
| test('transitive external dependencies are supported', async ({ page, turborepoNPM }) => { | ||
| const pageResponse = await page.goto( | ||
| new URL('/transitive-external-deps', turborepoNPM.url).href, | ||
| ) | ||
|
|
||
| expect(pageResponse?.status()).toBe(200) | ||
|
|
||
| await expect(page.getByTestId('dep-a-version')).toHaveText('3.10.1') | ||
| await expect(page.getByTestId('dep-b-version')).toHaveText('4.17.21') | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import depA from '@repo/dep-a' | ||
| import depB from '@repo/dep-b' | ||
|
|
||
| export default function TransitiveDeps() { | ||
| return ( | ||
| <body> | ||
| <ul> | ||
| <li> | ||
| dep-a uses lodash version 3.10.1 and we should see this version here:{' '} | ||
| <span data-testId="dep-a-version">{depA}</span> | ||
| </li> | ||
| <li> | ||
| dep-b uses lodash version 4.17.21 and we should see this version here:{' '} | ||
| <span data-testId="dep-b-version">{depB}</span> | ||
| </li> | ||
| </ul> | ||
| </body> | ||
| ) | ||
| } | ||
|
|
||
| // just to ensure this is rendered in runtime and not prerendered | ||
| export async function getServerSideProps() { | ||
| return { | ||
| props: {}, | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import lodash from 'lodash' | ||
|
|
||
| export default lodash.VERSION |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "name": "@repo/dep-a", | ||
| "version": "1.0.0", | ||
| "dependencies": { | ||
| "lodash": "3.10.1" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import lodash from 'lodash' | ||
|
|
||
| export default lodash.VERSION |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "name": "@repo/dep-b", | ||
| "version": "1.0.0", | ||
| "dependencies": { | ||
| "lodash": "4.17.21" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import depA from '@repo/dep-a' | ||
| import depB from '@repo/dep-b' | ||
|
|
||
| export default function TransitiveDeps() { | ||
| return ( | ||
| <body> | ||
| <ul> | ||
| <li> | ||
| dep-a uses lodash version 3.10.1 and we should see this version here:{' '} | ||
| <span data-testId="dep-a-version">{depA}</span> | ||
| </li> | ||
| <li> | ||
| dep-b uses lodash version 4.17.21 and we should see this version here:{' '} | ||
| <span data-testId="dep-b-version">{depB}</span> | ||
| </li> | ||
| </ul> | ||
| </body> | ||
| ) | ||
| } | ||
|
|
||
| // just to ensure this is rendered in runtime and not prerendered | ||
| export async function getServerSideProps() { | ||
| return { | ||
| props: {}, | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import lodash from 'lodash' | ||
|
|
||
| export default lodash.VERSION |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "name": "@repo/dep-a", | ||
| "version": "1.0.0", | ||
| "dependencies": { | ||
| "lodash": "3.10.1" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import lodash from 'lodash' | ||
|
|
||
| export default lodash.VERSION |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "name": "@repo/dep-b", | ||
| "version": "1.0.0", | ||
| "dependencies": { | ||
| "lodash": "4.17.21" | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is fix for next@canary change to ensure the "hashed" artificial
node_modulesare included in NF (see first part of description for more details including first screenshot)