TURBOPACK: expose node ecmascript chunk types and accessors#147
Conversation
There was a problem hiding this comment.
Code Review
This pull request exposes several internal Ecmascript Node.js chunk types and modules by making them public and adding various accessor methods. The review feedback highlights opportunities to reduce task graph overhead by removing the #[turbo_tasks::function] attribute from simple field accessors and cheap upcasts. Furthermore, the chunks_data method should be simplified to a synchronous implementation, as the current asynchronous approach and error handling are unnecessary for the operations being performed.
| #[turbo_tasks::function] | ||
| pub fn chunk(&self) -> Vc<Box<dyn Chunk>> { | ||
| Vc::upcast(*self.chunk) | ||
| } |
There was a problem hiding this comment.
The chunk method is a simple accessor that performs a cheap Vc::upcast. Marking it as a #[turbo_tasks::function] adds unnecessary overhead to the task graph (scheduling, memoization, etc.). It is better to provide it as a regular public method.
pub fn chunk(&self) -> Vc<Box<dyn Chunk>> {
Vc::upcast(*self.chunk)
}| #[turbo_tasks::function] | ||
| pub async fn chunks_data(&self) -> Result<Vc<ChunksData>> { | ||
| Ok(ChunkData::from_assets( | ||
| self.chunking_context.output_root().owned().await?, | ||
| *self.other_chunks, | ||
| )) | ||
| } |
There was a problem hiding this comment.
The chunks_data implementation can be simplified. ChunkData::from_assets accepts Vc arguments, so there is no need to await the output root or use .owned(). Removing the async and Result reduces overhead and aligns with the implementation in EcmascriptBrowserEvaluateChunk.
#[turbo_tasks::function]
pub fn chunks_data(&self) -> Vc<ChunksData> {
ChunkData::from_assets(self.chunking_context.output_root(), *self.other_chunks)
}| #[turbo_tasks::function] | ||
| pub fn evaluatable_assets(&self) -> Vc<EvaluatableAssets> { | ||
| *self.evaluatable_assets | ||
| } |
| #[turbo_tasks::function] | ||
| pub fn module_graph(&self) -> Vc<ModuleGraph> { | ||
| *self.module_graph | ||
| } |
| #[turbo_tasks::function] | ||
| pub fn chunking_context(&self) -> Vc<Box<dyn ChunkingContext>> { | ||
| Vc::upcast(*self.chunking_context) | ||
| } |
There was a problem hiding this comment.
Promote `EcmascriptBuildNodeChunk`, `EcmascriptBuildNodeEntryChunk`, and `EcmascriptBuildNodeRuntimeChunk` from `pub(crate)` to `pub`, and re-export them from `turbopack_nodejs`, mirroring the browser-side visibility. Add public accessors needed for downstream consumers (e.g. webpack-compatible stats generation): - `EcmascriptBuildNodeChunk::chunk()` — exposes the inner `Chunk` for chunk_items - `EcmascriptBuildNodeEntryChunk::chunks_data()` / `evaluatable_assets()` / `module_graph()` / `chunking_context()` — mirror `EcmascriptBrowserEvaluateChunk` No behavior changes inside turbopack-nodejs itself.
deaac1d to
4b33fa3
Compare
Promote `EcmascriptBuildNodeChunk`, `EcmascriptBuildNodeEntryChunk`, and `EcmascriptBuildNodeRuntimeChunk` from `pub(crate)` to `pub`, and re-export them from `turbopack_nodejs`, mirroring the browser-side visibility. Add public accessors needed for downstream consumers (e.g. webpack-compatible stats generation): - `EcmascriptBuildNodeChunk::chunk()` — exposes the inner `Chunk` for chunk_items - `EcmascriptBuildNodeEntryChunk::chunks_data()` / `evaluatable_assets()` / `module_graph()` / `chunking_context()` — mirror `EcmascriptBrowserEvaluateChunk` No behavior changes inside turbopack-nodejs itself.
Promote `EcmascriptBuildNodeChunk`, `EcmascriptBuildNodeEntryChunk`, and `EcmascriptBuildNodeRuntimeChunk` from `pub(crate)` to `pub`, and re-export them from `turbopack_nodejs`, mirroring the browser-side visibility. Add public accessors needed for downstream consumers (e.g. webpack-compatible stats generation): - `EcmascriptBuildNodeChunk::chunk()` — exposes the inner `Chunk` for chunk_items - `EcmascriptBuildNodeEntryChunk::chunks_data()` / `evaluatable_assets()` / `module_graph()` / `chunking_context()` — mirror `EcmascriptBrowserEvaluateChunk` No behavior changes inside turbopack-nodejs itself.
Promote `EcmascriptBuildNodeChunk`, `EcmascriptBuildNodeEntryChunk`, and `EcmascriptBuildNodeRuntimeChunk` from `pub(crate)` to `pub`, and re-export them from `turbopack_nodejs`, mirroring the browser-side visibility. Add public accessors needed for downstream consumers (e.g. webpack-compatible stats generation): - `EcmascriptBuildNodeChunk::chunk()` — exposes the inner `Chunk` for chunk_items - `EcmascriptBuildNodeEntryChunk::chunks_data()` / `evaluatable_assets()` / `module_graph()` / `chunking_context()` — mirror `EcmascriptBrowserEvaluateChunk` No behavior changes inside turbopack-nodejs itself.
Promote `EcmascriptBuildNodeChunk`, `EcmascriptBuildNodeEntryChunk`, and `EcmascriptBuildNodeRuntimeChunk` from `pub(crate)` to `pub`, and re-export them from `turbopack_nodejs`, mirroring the browser-side visibility. Add public accessors needed for downstream consumers (e.g. webpack-compatible stats generation): - `EcmascriptBuildNodeChunk::chunk()` — exposes the inner `Chunk` for chunk_items - `EcmascriptBuildNodeEntryChunk::chunks_data()` / `evaluatable_assets()` / `module_graph()` / `chunking_context()` — mirror `EcmascriptBrowserEvaluateChunk` No behavior changes inside turbopack-nodejs itself.
Promote `EcmascriptBuildNodeChunk`, `EcmascriptBuildNodeEntryChunk`, and `EcmascriptBuildNodeRuntimeChunk` from `pub(crate)` to `pub`, and re-export them from `turbopack_nodejs`, mirroring the browser-side visibility. Add public accessors needed for downstream consumers (e.g. webpack-compatible stats generation): - `EcmascriptBuildNodeChunk::chunk()` — exposes the inner `Chunk` for chunk_items - `EcmascriptBuildNodeEntryChunk::chunks_data()` / `evaluatable_assets()` / `module_graph()` / `chunking_context()` — mirror `EcmascriptBrowserEvaluateChunk` No behavior changes inside turbopack-nodejs itself.
Promote `EcmascriptBuildNodeChunk`, `EcmascriptBuildNodeEntryChunk`, and `EcmascriptBuildNodeRuntimeChunk` from `pub(crate)` to `pub`, and re-export them from `turbopack_nodejs`, mirroring the browser-side visibility. Add public accessors needed for downstream consumers (e.g. webpack-compatible stats generation): - `EcmascriptBuildNodeChunk::chunk()` — exposes the inner `Chunk` for chunk_items - `EcmascriptBuildNodeEntryChunk::chunks_data()` / `evaluatable_assets()` / `module_graph()` / `chunking_context()` — mirror `EcmascriptBrowserEvaluateChunk` No behavior changes inside turbopack-nodejs itself.
Promote `EcmascriptBuildNodeChunk`, `EcmascriptBuildNodeEntryChunk`, and `EcmascriptBuildNodeRuntimeChunk` from `pub(crate)` to `pub`, and re-export them from `turbopack_nodejs`, mirroring the browser-side visibility. Add public accessors needed for downstream consumers (e.g. webpack-compatible stats generation): - `EcmascriptBuildNodeChunk::chunk()` — exposes the inner `Chunk` for chunk_items - `EcmascriptBuildNodeEntryChunk::chunks_data()` / `evaluatable_assets()` / `module_graph()` / `chunking_context()` — mirror `EcmascriptBrowserEvaluateChunk` No behavior changes inside turbopack-nodejs itself.
Promote `EcmascriptBuildNodeChunk`, `EcmascriptBuildNodeEntryChunk`, and `EcmascriptBuildNodeRuntimeChunk` from `pub(crate)` to `pub`, and re-export them from `turbopack_nodejs`, mirroring the browser-side visibility. Add public accessors needed for downstream consumers (e.g. webpack-compatible stats generation): - `EcmascriptBuildNodeChunk::chunk()` — exposes the inner `Chunk` for chunk_items - `EcmascriptBuildNodeEntryChunk::chunks_data()` / `evaluatable_assets()` / `module_graph()` / `chunking_context()` — mirror `EcmascriptBrowserEvaluateChunk` No behavior changes inside turbopack-nodejs itself.
Promote `EcmascriptBuildNodeChunk`, `EcmascriptBuildNodeEntryChunk`, and `EcmascriptBuildNodeRuntimeChunk` from `pub(crate)` to `pub`, and re-export them from `turbopack_nodejs`, mirroring the browser-side visibility. Add public accessors needed for downstream consumers (e.g. webpack-compatible stats generation): - `EcmascriptBuildNodeChunk::chunk()` — exposes the inner `Chunk` for chunk_items - `EcmascriptBuildNodeEntryChunk::chunks_data()` / `evaluatable_assets()` / `module_graph()` / `chunking_context()` — mirror `EcmascriptBrowserEvaluateChunk` No behavior changes inside turbopack-nodejs itself.
Promote `EcmascriptBuildNodeChunk`, `EcmascriptBuildNodeEntryChunk`, and `EcmascriptBuildNodeRuntimeChunk` from `pub(crate)` to `pub`, and re-export them from `turbopack_nodejs`, mirroring the browser-side visibility. Add public accessors needed for downstream consumers (e.g. webpack-compatible stats generation): - `EcmascriptBuildNodeChunk::chunk()` — exposes the inner `Chunk` for chunk_items - `EcmascriptBuildNodeEntryChunk::chunks_data()` / `evaluatable_assets()` / `module_graph()` / `chunking_context()` — mirror `EcmascriptBrowserEvaluateChunk` No behavior changes inside turbopack-nodejs itself.
Promote `EcmascriptBuildNodeChunk`, `EcmascriptBuildNodeEntryChunk`, and `EcmascriptBuildNodeRuntimeChunk` from `pub(crate)` to `pub`, and re-export them from `turbopack_nodejs`, mirroring the browser-side visibility. Add public accessors needed for downstream consumers (e.g. webpack-compatible stats generation): - `EcmascriptBuildNodeChunk::chunk()` — exposes the inner `Chunk` for chunk_items - `EcmascriptBuildNodeEntryChunk::chunks_data()` / `evaluatable_assets()` / `module_graph()` / `chunking_context()` — mirror `EcmascriptBrowserEvaluateChunk` No behavior changes inside turbopack-nodejs itself.
Promote `EcmascriptBuildNodeChunk`, `EcmascriptBuildNodeEntryChunk`, and `EcmascriptBuildNodeRuntimeChunk` from `pub(crate)` to `pub`, and re-export them from `turbopack_nodejs`, mirroring the browser-side visibility. Add public accessors needed for downstream consumers (e.g. webpack-compatible stats generation): - `EcmascriptBuildNodeChunk::chunk()` — exposes the inner `Chunk` for chunk_items - `EcmascriptBuildNodeEntryChunk::chunks_data()` / `evaluatable_assets()` / `module_graph()` / `chunking_context()` — mirror `EcmascriptBrowserEvaluateChunk` No behavior changes inside turbopack-nodejs itself.
Promote
EcmascriptBuildNodeChunk,EcmascriptBuildNodeEntryChunk, andEcmascriptBuildNodeRuntimeChunkfrompub(crate)topub, and re-export them fromturbopack_nodejs, mirroring the browser-side visibility.Add public accessors needed for downstream consumers (e.g. webpack-compatible stats generation):
EcmascriptBuildNodeChunk::chunk()— exposes the innerChunkfor chunk_itemsEcmascriptBuildNodeEntryChunk::chunks_data()/evaluatable_assets()/module_graph()/chunking_context()— mirrorEcmascriptBrowserEvaluateChunkNo behavior changes inside turbopack-nodejs itself.