-
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
base: main
Are you sure you want to change the base?
Conversation
📊 Package size report 0.03%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
774696c to
ab01d87
Compare
This reverts commit 8d10761.
…outside of project workspace
f0746ea to
d58ed23
Compare
| // 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 [email protected] | ||
| // 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, | ||
| }) | ||
| } |
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_modules are included in NF (see first part of description for more details including first screenshot)
| 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 | ||
| if (existsSync(rootSrcDir) && ctx.standaloneRootDir !== ctx.standaloneDir) { | ||
| promises.push( | ||
| cp(rootSrcDir, rootDestDir, { recursive: true, verbatimSymlinks: true, filter }).then(() => | ||
| recreateNodeModuleSymlinks(resolve('node_modules'), rootDestDir), | ||
| ), | ||
| ) |
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 change is for the edge case encountered after I expanded test case (last paragraph + second screenshot). It starts with change of "traversing" from ctx.standaloneDir (which in monorepos will be standalone/path/to/workspace) to ctx.standaloneRootDir (just standalone).
and required adjustments to logic to still handle some special cases (like skipping ctx.nextDistDir here and calling recreateNodeModuleSymlinks, which now no longer have explicit special case for monorepo and instead have streamlined handling for it)
Description
Adjustments to handle recent e2e failures on next@canary ( https://github.com/opennextjs/opennextjs-netlify/actions/runs/19727042798 ).
Some details:
It seems like imports in pages router with turbopack builds now generate additional
node_modulesdirectory with symlinks.We currently don't handle
standalone/.next/node_modules(marked with red on above screenshot). We only handlestandalone/node_modules(marked with green). This adds handling for "new" node_modules location, preserving hierarchy (so no merging of them, we will havenode_modulesand.next/node_modulesin lambda)It does look like vercel/next.js@7905cb4#diff-86d6014a082abde94fef6b35552605715afe6eb9e7e467ac931d2f153cd3b934 is what introduced change. It does seem to me that that there is some kind of bug there to be honest, because we are hitting case of producing those hashed symlinks on DIRECT dependencies that we don't even declare as external in config and because of that I'm expanding our tests (following tests added in that commit) to ensure we have coverage in future as well in case it is indeed a bug that we fell into this new behavior despite seemingly not meeting criteria for it.
Additionally ... while expanding test fixture I've hit yet another unhandled edge case, where we were not copying some some top-level directories from root of standalone directory (marked directory missing in NF)