Skip to content

fix: glob subdirectories in server/chunks to support turbopack builds #2987

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

Merged
merged 6 commits into from
Jul 10, 2025

Conversation

timneutkens
Copy link
Contributor

@timneutkens timneutkens commented Jul 5, 2025

Description

TLDR: This change makes next build --turbopack work on Netlify

Was investigating why next build --turbopack fails when deploying on Netlify:

  • Turns out this glob is targeting .next/server/chunks/* instead of .next/server/chunks/**/* in the standalone dir.
  • With Turbopack the output can have additional subdirectories (could have happened with webpack too if the output folder was ever changed).
  • Upon further investigation it seems that you'd want to collect all of .next/standalone/.next not just .next/* and .next/server/**/*
  • I don't think there should be a limit on .js because that would exclude e.g. sourcemaps which are .js.map files.

For now to verify if there's a specific reason the subdirectories were targeted I've changed the glob to .next/server/**/* in order to see if any tests fail.

Documentation

Tests

TODO: This needs a next build --turbopack app.

You can test this change yourself like so:

  1. TODO

Relevant links (GitHub issues, etc.) or a picture of cute animal

@timneutkens
Copy link
Contributor Author

I've added a test fixture and copied the simple test suite. However I'm unable to run it successfully locally, it seems I may be missing something, let me know if you're able to run it 🙏

@timneutkens timneutkens marked this pull request as ready for review July 6, 2025 13:25
@timneutkens timneutkens requested a review from a team as a code owner July 6, 2025 13:25
@pieh
Copy link
Contributor

pieh commented Jul 8, 2025

Firstly, huge thanks for PR!

  • Turns out this glob is targeting .next/server/chunks/* instead of .next/server/chunks/**/* in the standalone dir.
  • With Turbopack the output can have additional subdirectories (could have happened with webpack too if the output folder was ever changed).

Awesome find, thank you!

  • Upon further investigation it seems that you'd want to collect all of .next/standalone/.next not just .next/* and .next/server/**/*
  • I don't think there should be a limit on .js because that would exclude e.g. sourcemaps which are .js.map files.

Limiting to .js in .next/server/app|pages was done to not bundle .html, .rsc, .meta, .json for content prerendered at build, because we do have custom CacheHandler that doesn't use filesystem for storage so those files would needlesly grow size of produced function without being actually used. Limiting to .js like we do now is probably too restrictive overall and instead probably we should exclude those specific targeted things we do have alternative handling for.

Expanding a bit on selective dirs globbing - it was done to try to limit chances of exceeding function max size. We do generate single function for all routes today (hopefully to be changed with adapters), so it's easier in this setup to go over the edge than with function per route. This makes this change a bit risky for us and instead I'd like to suggest following:

Could we keep this change to only target turbopack builds needs for right now? My understanding just from reading first 2 points is that it could be addressed by adjusting our server/chunks/* (and server/edge-chunks/*?) pattern to allow sub directories.

Then I would follow up with some ~telemetry addition to asses impact of doing the change in this PR almost as-is (just with added exclussion for default filesystem CacheHandler entries) on size of produced lambda to understand if this is generally safe change for us to roll out or would this result in failures (function size too large) for some users.

I'm happy to take over getting this over the finish line if you wish, just let me know what your thoughts are on this.

@pieh
Copy link
Contributor

pieh commented Jul 8, 2025

Limiting to .js in .next/server/app|pages was done to not bundle .html, .rsc, .meta, .json for content prerendered at build, because we do have custom CacheHandler that doesn't use filesystem for storage so those files would needlesly grow size of produced function without being actually used. Limiting to .js like we do now is probably too restrictive overall and instead probably we should exclude those specific targeted things we do have alternative handling for.

I was checking out failing test and seems to me that those failures are actually caused by including those CacheHandler entries in function without making adjustments elsewhere. To be fair the handling is a bit brittle and not resiliant to changes like that, but it was operating under assumption that .html files would not be bundled and our fs/promises.readFile would throw -

try {
// Attempt to read from the disk
// important to use the `import * as fs from 'fs'` here to not end up in a endless loop
return await ofs.readFile(path, options)
} catch (error) {
// only try to get .html files from the blob store
if (typeof path === 'string' && path.endsWith('.html')) {
const cacheStore = getMemoizedKeyValueStoreBackedByRegionalBlobStore()
const relPath = relative(resolve('.next/server/pages'), path)
const file = await cacheStore.get<HtmlBlob>(relPath, 'staticHtml.get')
if (file !== null) {
if (file.isFullyStaticPage) {
const requestContext = getRequestContext()
// On server initialization Next.js attempt to preload all pages
// which might result in reading .html files from the file system
// for fully static pages. We don't want to capture those cases.
// Note that Next.js does NOT cache read html files so on actual requests
// that those will be served, it will read those AGAIN and then we do
// want to capture fact of reading them.
const { initializingServer } = initAsyncLocalStorage.getStore() ?? {}
if (!initializingServer && requestContext) {
requestContext.usedFsReadForNonFallback = true
}
}
return file.html
}
}
throw error
}

I'll attempt to open PR with optimistic change that scales back the globbing part to what seems like turbopack builds specific needs to verify that is indeed what's needed

@timneutkens
Copy link
Contributor Author

Only matching chunks/**/* would work yeah 👍

@pieh pieh changed the title Glob all server files instead of specific subdirectories fix: glob subdirectories in server/chunks to support turbopack builds Jul 10, 2025
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank You!

@pieh pieh merged commit e7bf18f into opennextjs:main Jul 10, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants