-
Notifications
You must be signed in to change notification settings - Fork 30.1k
[turbopack] Return cached Promise from __turbopack_load_by_url__
#81663
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
b28994c
407b2e6
cf68768
a7ceabc
62ac164
2f72147
71738a2
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 |
|---|---|---|
|
|
@@ -346,6 +346,13 @@ export async function createHotReloaderTurbopack( | |
| p.startsWith('server/app') | ||
| ) | ||
|
|
||
| // Edge uses the browser runtime which already disposes chunks individually. | ||
| // TODO: process.env.NEXT_RUNTIME is 'nodejs' even though Node.js runtime is not used. | ||
| if ('__turbopack_clear_chunk_cache__' in globalThis) { | ||
| ;(globalThis as any).__turbopack_clear_chunk_cache__() | ||
| } | ||
|
|
||
| // TODO: Stop re-evaluating React Client once it relies on Turbopack's chunk cache. | ||
|
Member
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. needs facebook/react#33792 |
||
| if (hasAppPaths) { | ||
| deleteFromRequireCache( | ||
| require.resolve( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -93,6 +93,13 @@ function loadChunk(chunkData: ChunkData, source?: SourceInfo): void { | |||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const loadedChunks = new Set<ChunkPath>() | ||||||||||||
| const unsupportedLoadChunk = Promise.resolve(undefined) | ||||||||||||
| const loadedChunk = Promise.resolve(undefined) | ||||||||||||
| const chunkCache = new Map<ChunkPath, Promise<any> | null>() | ||||||||||||
|
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. I understand you want to avoid holding onto extra promises, but it does seem a bit confusing (evidenced by Vade's (the AI) wrong comment) that Why not just store If you don't want to do that, consider using a sentinel value of
Member
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. Makes sense. I'll do that in a follow-up |
||||||||||||
|
|
||||||||||||
| function clearChunkCache() { | ||||||||||||
| chunkCache.clear() | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| function loadChunkPath(chunkPath: ChunkPath, source?: SourceInfo): void { | ||||||||||||
| if (!isJs(chunkPath)) { | ||||||||||||
|
|
@@ -136,21 +143,10 @@ function loadChunkPath(chunkPath: ChunkPath, source?: SourceInfo): void { | |||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| async function loadChunkAsync( | ||||||||||||
| async function loadChunkAsyncUncached( | ||||||||||||
| source: SourceInfo, | ||||||||||||
| chunkData: ChunkData | ||||||||||||
| ): Promise<any> { | ||||||||||||
| const chunkPath = typeof chunkData === 'string' ? chunkData : chunkData.path | ||||||||||||
| if (!isJs(chunkPath)) { | ||||||||||||
| // We only support loading JS chunks in Node.js. | ||||||||||||
| // This branch can be hit when trying to load a CSS chunk. | ||||||||||||
| return | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (loadedChunks.has(chunkPath)) { | ||||||||||||
| return | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| chunkPath: ChunkPath | ||||||||||||
| ): Promise<void> { | ||||||||||||
| const resolved = path.resolve(RUNTIME_ROOT, chunkPath) | ||||||||||||
|
|
||||||||||||
| try { | ||||||||||||
|
|
@@ -187,7 +183,6 @@ async function loadChunkAsync( | |||||||||||
| } | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| loadedChunks.add(chunkPath) | ||||||||||||
| } catch (e) { | ||||||||||||
| let errorMessage = `Failed to load chunk ${chunkPath}` | ||||||||||||
|
|
||||||||||||
|
|
@@ -201,7 +196,30 @@ async function loadChunkAsync( | |||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| async function loadChunkAsyncByUrl(source: SourceInfo, chunkUrl: string) { | ||||||||||||
| function loadChunkAsync( | ||||||||||||
| source: SourceInfo, | ||||||||||||
| chunkData: ChunkData | ||||||||||||
| ): Promise<any> { | ||||||||||||
| const chunkPath = typeof chunkData === 'string' ? chunkData : chunkData.path | ||||||||||||
| if (!isJs(chunkPath)) { | ||||||||||||
| // We only support loading JS chunks in Node.js. | ||||||||||||
| // This branch can be hit when trying to load a CSS chunk. | ||||||||||||
| return unsupportedLoadChunk | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| let entry = chunkCache.get(chunkPath) | ||||||||||||
| if (entry === undefined) { | ||||||||||||
| const resolve = chunkCache.set.bind(chunkCache, chunkPath, null) | ||||||||||||
|
Contributor
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. The chunk cache logic in the Node.js runtime has a critical bug where successful chunk loads set the cache entry to View DetailsAnalysisIn the
This means successful chunk loads are not properly cached. The intent appears to be marking the cache entry as "completed" when the chunk finishes loading, but instead it's being set to RecommendationThe resolve function should mark the cache entry as completed rather than setting it to const resolve = () => chunkCache.set(chunkPath, null)Or better yet, implement a proper completion marker system where 👍 or 👎 to improve Vade. |
||||||||||||
| // A new Promise ensures callers that don't handle rejection will still trigger one unhandled rejection. | ||||||||||||
| // Handling the rejection will not trigger unhandled rejections. | ||||||||||||
| entry = loadChunkAsyncUncached(source, chunkPath).then(resolve) | ||||||||||||
| chunkCache.set(chunkPath, entry) | ||||||||||||
| } | ||||||||||||
| // TODO: Return an instrumented Promise that React can use instead of relying on referential equality. | ||||||||||||
| return entry === null ? loadedChunk : entry | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| function loadChunkAsyncByUrl(source: SourceInfo, chunkUrl: string) { | ||||||||||||
| const path = url.fileURLToPath(new URL(chunkUrl, RUNTIME_ROOT)) as ChunkPath | ||||||||||||
| return loadChunkAsync(source, path) | ||||||||||||
| } | ||||||||||||
|
|
@@ -363,6 +381,9 @@ function isJs(chunkUrlOrPath: ChunkUrl | ChunkPath): boolean { | |||||||||||
| return regexJsUrl.test(chunkUrlOrPath) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // For hot-reloader | ||||||||||||
| ;(globalThis as any).__turbopack_clear_chunk_cache__ = clearChunkCache | ||||||||||||
|
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. I think it's better to declare a global in typescript? e.g. next.js/packages/next/src/client/dev/hot-reloader/pages/hot-reloader-pages.ts Lines 63 to 67 in d0ea712
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. We do not want to expose globals in the Turbopack runtime. You can expose a method on the Turbopack context instead and expose it as global from next.js
Member
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. We settled on doing this in a follow-up. It'll be a continuation of
Member
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. |
||||||||||||
|
|
||||||||||||
| module.exports = { | ||||||||||||
| getOrInstantiateRuntimeModule, | ||||||||||||
| loadChunk, | ||||||||||||
|
|
||||||||||||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
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.
Edge doesn't implement
__turbopack_load_by_url__so we can ignore for now. But this needs clarification in a follow-up since it's confusing thatprocess.env.NEXT_RUNTIMEis'nodejs'while Turbopack doesn't use the Node.js runtime.