ChunkingContext refactor pt. 1 #4397

alexkirsz · 2023-03-30T12:05:15Z

Next.js side: vercel/next.js#47693

Description

This is part one of a larger ChunkingContext refactor.

In this episode, we:

Move the runtime and evaluation code off of the entry JS chunk and into a dedicated chunk. This means that, within a chunk group, instead of having one chunk that contains modules + runtime code and chunks that only contain modules, we'll have chunks that only contain modules and one chunk that only contains runtime code.
Start a larger enterprise of differentiating between "intermediate" chunks (ChunkVcs), and "output" chunks (raw AssetVcs). Before, we used EcmascriptChunkVcs to represent both intermediate chunks that would never actually be created (optimized away) AND final chunks that would be served or written to disk. Now, EcmascriptChunkVcs are only intermediate assets, and DevEcmascriptChunkVcs are the final chunks.
Move a few modules from turbopack-core to turbopack-dev, as their existence (or the implementation) is only meaningful in a dev context (ChunkList and Manifest*, the latter of which depends on the chunk list for now).

Things left to figure out:

What does the Chunk trait represent here? We no longer use .chunking_context(), and .path() is redundant with Asset::path. However, it no longer makes sense for Chunks to be Assets as their content is determined by the chunking context. Should we have two traits, one for intermediate chunks and one for output chunks?
Should EvalutatedEntryVc just be ChunkableAssetVc? We don't need all of the trait methods of either, just an ident and a way to convert to a chunk.

The Chunk trait could look like this:

trait Chunk {
  fn ident(&self) -> IdentVc;
  fn parallel_chunks(&self) -> ChunksVc;
  fn references(&self) -> AssetReferencesVc;
}

Things that will happen in the next part(s):

The ChunkGroupVc::evaluated logic will move into turbopack-dev, as it doesn't make sense in a build context. This will essentially make ChunkGroupVc obsolete: the only thing we need it to do is to compute all parallel chunks, but I'm thinking this should be a method on the Chunk trait instead.

Things I'll leave for future refactors:

Figure out the whole Input, Intermediate, and Output asset story.

Testing Instructions

Snapshots + Next integration tests
fix WEB-815 (link)

vercel · 2023-03-30T12:05:26Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
examples-basic-web	🔄 Building (Inspect)			Apr 4, 2023 2:24pm
examples-vite-web	🔄 Building (Inspect)			Apr 4, 2023 2:24pm

9 Ignored Deployments

Name	Status	Preview	Updated (UTC)
examples-cra-web	⬜️ Ignored (Inspect)		Apr 4, 2023 2:24pm
examples-designsystem-docs	⬜️ Ignored (Inspect)		Apr 4, 2023 2:24pm
examples-gatsby-web	⬜️ Ignored (Inspect)		Apr 4, 2023 2:24pm
examples-kitchensink-blog	⬜️ Ignored (Inspect)		Apr 4, 2023 2:24pm
examples-native-web	⬜️ Ignored (Inspect)		Apr 4, 2023 2:24pm
examples-nonmonorepo	⬜️ Ignored (Inspect)		Apr 4, 2023 2:24pm
examples-svelte-web	⬜️ Ignored (Inspect)		Apr 4, 2023 2:24pm
examples-tailwind-web	⬜️ Ignored (Inspect)		Apr 4, 2023 2:24pm
turbo-site	⬜️ Ignored (Inspect)	Visit Preview	Apr 4, 2023 2:24pm

github-actions · 2023-03-30T12:20:36Z

✅ This changes can build next-swc

github-actions · 2023-03-30T12:27:07Z

🟢 CI successful 🟢

Thanks

alexkirsz · 2023-03-30T12:56:38Z

crates/turbopack-tests/tests/snapshot.rs

@@ -305,13 +311,17 @@ async fn walk_asset(
        return Ok(());
    }

-    diff(path, asset.content()).await?;
+    if path.await?.is_inside(&*output_path) {


This fixes an issue where we would be diffing against assets that aren't in the output directory.

I think this is the reason for the large negative line count of this PR.

alexkirsz · 2023-03-30T13:29:43Z

crates/turbopack-core/src/chunk/mod.rs

+            .try_join()
+            .await?
+            .into_iter()
+            .collect();


This is a big difference: before, we were putting evaluated entries inside of the main entries of the "evaluated chunk". Now, they are considered separate chunks to begin with, and it's up to the optimizer to merge them.

crates/turbopack-dev/src/ecmascript/chunk.rs

crates/turbopack-ecmascript/src/source_map.rs

crates/turbopack-core/src/chunk/evaluate.rs

sokra · 2023-03-30T14:43:16Z

crates/turbopack-dev/src/ecmascript/evaluate/chunk.rs

+                    if let Some(placeable) = EcmascriptChunkPlaceableVc::resolve_from(entry).await?
+                    {
+                        Ok(Some(
+                            placeable
+                                .as_chunk_item(chunking_context.into())
+                                .id()
+                                .await?,
+                        ))
+                    } else {
+                        Ok(None)
+                    }


This can become a trait method in EvaluatableAsset to avoid the cast here.

fn evaluated_module_id(&self) -> ModuleIdVc

sounds good for the start...

The purpose of the EvaluatableAsset trait is mostly to act as a marker trait. It's up to the implementor to actually decide where it can be evaluated, by implementing specific traits such as EcmascriptChunkPlaceable. For instance, we could have Wasm evaluated entries, which we might want to handle differently.

sokra

Might require some manual tests for:

Introspection
SourceMaps
HMR
source maps in runtime error overlay

This is the Next.js side of vercel/turborepo#4397 --------- Co-authored-by: Tobias Koppers <tobias.koppers@googlemail.com> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

### Description Next.js side: vercel/next.js#47961 Previous episode: #4397 In this episode, we: 1. Get rid of `ChunkGroupVc`. `ChunkGroupVc` previously took care of: a. optimizing chunks; b. adding additional chunks, such as the "evaluate" chunk, which evaluates runtime entries. Both of these use cases are a concern of the chunking context (i.e. it would differ between dev and build), hence they're now handled by the chunking context. 2. Move the chunk list registration logic into a different chunk. The chunk list was previously a bit leaky as it required the Next.js code to instantiate it manually. There were also a bunch of other instantiations in the code which differed ever so slightly. The chunk list registration code is now part of a separate chunk, which is only added by the dev chunking context. ### Testing Instructions Snapshots + Next dev tests + Manual tests fix WEB-822

Next.js side: #47693 ### Description This is part one of a larger ChunkingContext refactor. In this episode, we: 1. Move the runtime and evaluation code off of the entry JS chunk and into a dedicated chunk. This means that, within a chunk group, instead of having one chunk that contains modules + runtime code and chunks that only contain modules, we'll have chunks that only contain modules and one chunk that only contains runtime code. 2. Start a larger enterprise of differentiating between "intermediate" chunks (`ChunkVc`s), and "output" chunks (raw `AssetVc`s). Before, we used `EcmascriptChunkVc`s to represent both intermediate chunks that would never actually be created (optimized away) AND final chunks that would be served or written to disk. Now, `EcmascriptChunkVc`s are only intermediate assets, and `DevEcmascriptChunkVc`s are the final chunks. 3. Move a few modules from `turbopack-core` to `turbopack-dev`, as their existence (or the implementation) is only meaningful in a dev context (`ChunkList` and `Manifest*`, the latter of which depends on the chunk list for now). Things left to figure out: 1. What does the `Chunk` trait represent here? We no longer use `.chunking_context()`, and `.path()` is redundant with `Asset::path`. However, it no longer makes sense for `Chunk`s to be `Asset`s as their content is determined by the chunking context. Should we have two traits, one for intermediate chunks and one for output chunks? 2. Should `EvalutatedEntryVc` just be `ChunkableAssetVc`? We don't need *all* of the trait methods of either, just an ident and a way to convert to a chunk. The `Chunk` trait could look like this: ```rust trait Chunk { fn ident(&self) -> IdentVc; fn parallel_chunks(&self) -> ChunksVc; fn references(&self) -> AssetReferencesVc; } ``` Things that will happen in the next part(s): 1. The `ChunkGroupVc::evaluated` logic will move into `turbopack-dev`, as it doesn't make sense in a build context. This will essentially make `ChunkGroupVc` obsolete: the only thing we need it to do is to compute all parallel chunks, but I'm thinking this should be a method on the `Chunk` trait instead. Things I'll leave for future refactors: * Figure out the whole Input, Intermediate, and Output asset story. ### Testing Instructions Snapshots + Next integration tests fix WEB-815 ([link](https://linear.app/vercel/issue/WEB-815))

### Description Next.js side: #47961 Previous episode: vercel/turborepo#4397 In this episode, we: 1. Get rid of `ChunkGroupVc`. `ChunkGroupVc` previously took care of: a. optimizing chunks; b. adding additional chunks, such as the "evaluate" chunk, which evaluates runtime entries. Both of these use cases are a concern of the chunking context (i.e. it would differ between dev and build), hence they're now handled by the chunking context. 2. Move the chunk list registration logic into a different chunk. The chunk list was previously a bit leaky as it required the Next.js code to instantiate it manually. There were also a bunch of other instantiations in the code which differed ever so slightly. The chunk list registration code is now part of a separate chunk, which is only added by the dev chunking context. ### Testing Instructions Snapshots + Next dev tests + Manual tests fix WEB-822

Next.js side: #47693 ### Description This is part one of a larger ChunkingContext refactor. In this episode, we: 1. Move the runtime and evaluation code off of the entry JS chunk and into a dedicated chunk. This means that, within a chunk group, instead of having one chunk that contains modules + runtime code and chunks that only contain modules, we'll have chunks that only contain modules and one chunk that only contains runtime code. 2. Start a larger enterprise of differentiating between "intermediate" chunks (`ChunkVc`s), and "output" chunks (raw `AssetVc`s). Before, we used `EcmascriptChunkVc`s to represent both intermediate chunks that would never actually be created (optimized away) AND final chunks that would be served or written to disk. Now, `EcmascriptChunkVc`s are only intermediate assets, and `DevEcmascriptChunkVc`s are the final chunks. 3. Move a few modules from `turbopack-core` to `turbopack-dev`, as their existence (or the implementation) is only meaningful in a dev context (`ChunkList` and `Manifest*`, the latter of which depends on the chunk list for now). Things left to figure out: 1. What does the `Chunk` trait represent here? We no longer use `.chunking_context()`, and `.path()` is redundant with `Asset::path`. However, it no longer makes sense for `Chunk`s to be `Asset`s as their content is determined by the chunking context. Should we have two traits, one for intermediate chunks and one for output chunks? 2. Should `EvalutatedEntryVc` just be `ChunkableAssetVc`? We don't need *all* of the trait methods of either, just an ident and a way to convert to a chunk. The `Chunk` trait could look like this: ```rust trait Chunk { fn ident(&self) -> IdentVc; fn parallel_chunks(&self) -> ChunksVc; fn references(&self) -> AssetReferencesVc; } ``` Things that will happen in the next part(s): 1. The `ChunkGroupVc::evaluated` logic will move into `turbopack-dev`, as it doesn't make sense in a build context. This will essentially make `ChunkGroupVc` obsolete: the only thing we need it to do is to compute all parallel chunks, but I'm thinking this should be a method on the `Chunk` trait instead. Things I'll leave for future refactors: * Figure out the whole Input, Intermediate, and Output asset story. ### Testing Instructions Snapshots + Next integration tests fix WEB-815 ([link](https://linear.app/vercel/issue/WEB-815))

### Description Next.js side: #47961 Previous episode: vercel/turborepo#4397 In this episode, we: 1. Get rid of `ChunkGroupVc`. `ChunkGroupVc` previously took care of: a. optimizing chunks; b. adding additional chunks, such as the "evaluate" chunk, which evaluates runtime entries. Both of these use cases are a concern of the chunking context (i.e. it would differ between dev and build), hence they're now handled by the chunking context. 2. Move the chunk list registration logic into a different chunk. The chunk list was previously a bit leaky as it required the Next.js code to instantiate it manually. There were also a bunch of other instantiations in the code which differed ever so slightly. The chunk list registration code is now part of a separate chunk, which is only added by the dev chunking context. ### Testing Instructions Snapshots + Next dev tests + Manual tests fix WEB-822

Next.js side: #47693 ### Description This is part one of a larger ChunkingContext refactor. In this episode, we: 1. Move the runtime and evaluation code off of the entry JS chunk and into a dedicated chunk. This means that, within a chunk group, instead of having one chunk that contains modules + runtime code and chunks that only contain modules, we'll have chunks that only contain modules and one chunk that only contains runtime code. 2. Start a larger enterprise of differentiating between "intermediate" chunks (`ChunkVc`s), and "output" chunks (raw `AssetVc`s). Before, we used `EcmascriptChunkVc`s to represent both intermediate chunks that would never actually be created (optimized away) AND final chunks that would be served or written to disk. Now, `EcmascriptChunkVc`s are only intermediate assets, and `DevEcmascriptChunkVc`s are the final chunks. 3. Move a few modules from `turbopack-core` to `turbopack-dev`, as their existence (or the implementation) is only meaningful in a dev context (`ChunkList` and `Manifest*`, the latter of which depends on the chunk list for now). Things left to figure out: 1. What does the `Chunk` trait represent here? We no longer use `.chunking_context()`, and `.path()` is redundant with `Asset::path`. However, it no longer makes sense for `Chunk`s to be `Asset`s as their content is determined by the chunking context. Should we have two traits, one for intermediate chunks and one for output chunks? 2. Should `EvalutatedEntryVc` just be `ChunkableAssetVc`? We don't need *all* of the trait methods of either, just an ident and a way to convert to a chunk. The `Chunk` trait could look like this: ```rust trait Chunk { fn ident(&self) -> IdentVc; fn parallel_chunks(&self) -> ChunksVc; fn references(&self) -> AssetReferencesVc; } ``` Things that will happen in the next part(s): 1. The `ChunkGroupVc::evaluated` logic will move into `turbopack-dev`, as it doesn't make sense in a build context. This will essentially make `ChunkGroupVc` obsolete: the only thing we need it to do is to compute all parallel chunks, but I'm thinking this should be a method on the `Chunk` trait instead. Things I'll leave for future refactors: * Figure out the whole Input, Intermediate, and Output asset story. ### Testing Instructions Snapshots + Next integration tests fix WEB-815 ([link](https://linear.app/vercel/issue/WEB-815))

### Description Next.js side: #47961 Previous episode: vercel/turborepo#4397 In this episode, we: 1. Get rid of `ChunkGroupVc`. `ChunkGroupVc` previously took care of: a. optimizing chunks; b. adding additional chunks, such as the "evaluate" chunk, which evaluates runtime entries. Both of these use cases are a concern of the chunking context (i.e. it would differ between dev and build), hence they're now handled by the chunking context. 2. Move the chunk list registration logic into a different chunk. The chunk list was previously a bit leaky as it required the Next.js code to instantiate it manually. There were also a bunch of other instantiations in the code which differed ever so slightly. The chunk list registration code is now part of a separate chunk, which is only added by the dev chunking context. ### Testing Instructions Snapshots + Next dev tests + Manual tests fix WEB-822

alexkirsz requested a review from a team as a code owner March 30, 2023 12:05

turbo-orchestrator bot added the team: turbopack label Mar 30, 2023

alexkirsz force-pushed the alexkirsz/web-815-chunk-refactor-1 branch from ef7510b to 442ee27 Compare March 30, 2023 12:12

alexkirsz commented Mar 30, 2023

View reviewed changes

alexkirsz force-pushed the alexkirsz/web-815-chunk-refactor-1 branch from 442ee27 to c5fb7df Compare March 30, 2023 14:02

alexkirsz mentioned this pull request Mar 30, 2023

ChunkingContext refactor pt. 1 vercel/next.js#47693

Merged

sokra reviewed Mar 30, 2023

View reviewed changes

crates/turbopack-dev/src/ecmascript/chunk.rs Show resolved Hide resolved

sokra reviewed Mar 30, 2023

View reviewed changes

crates/turbopack-ecmascript/src/source_map.rs Outdated Show resolved Hide resolved

sokra reviewed Mar 30, 2023

View reviewed changes

crates/turbopack-core/src/chunk/evaluate.rs Outdated Show resolved Hide resolved

sokra reviewed Mar 30, 2023

View reviewed changes

sokra approved these changes Mar 30, 2023

View reviewed changes

alexkirsz force-pushed the alexkirsz/web-815-chunk-refactor-1 branch 3 times, most recently from a5c7d00 to f66e114 Compare April 4, 2023 11:59

vercel bot deployed to Preview – turbo-site April 4, 2023 12:00 View deployment

alexkirsz force-pushed the alexkirsz/web-815-chunk-refactor-1 branch 4 times, most recently from 00516ca to 023b3be Compare April 4, 2023 13:44

ChunkingContext refactor pt. 1

09a006c

alexkirsz force-pushed the alexkirsz/web-815-chunk-refactor-1 branch from 023b3be to f3f0cb0 Compare April 4, 2023 14:21

Update snapshots

c60e1f7

alexkirsz force-pushed the alexkirsz/web-815-chunk-refactor-1 branch from f3f0cb0 to c60e1f7 Compare April 4, 2023 14:24

alexkirsz merged commit cde4882 into main Apr 4, 2023

alexkirsz deleted the alexkirsz/web-815-chunk-refactor-1 branch April 4, 2023 14:56

alexkirsz mentioned this pull request Apr 4, 2023

Chunking Refactor pt. 2 #4450

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ChunkingContext refactor pt. 1 #4397

ChunkingContext refactor pt. 1 #4397

alexkirsz commented Mar 30, 2023 •

edited

Loading

vercel bot commented Mar 30, 2023 •

edited

Loading

github-actions bot commented Mar 30, 2023

github-actions bot commented Mar 30, 2023 •

edited

Loading

alexkirsz Mar 30, 2023

alexkirsz Mar 30, 2023

alexkirsz Mar 30, 2023

sokra Mar 30, 2023

alexkirsz Mar 31, 2023

sokra left a comment

ChunkingContext refactor pt. 1 #4397

ChunkingContext refactor pt. 1 #4397

Conversation

alexkirsz commented Mar 30, 2023 • edited Loading

Description

Testing Instructions

vercel bot commented Mar 30, 2023 • edited Loading

github-actions bot commented Mar 30, 2023

github-actions bot commented Mar 30, 2023 • edited Loading

🟢 CI successful 🟢

alexkirsz Mar 30, 2023

Choose a reason for hiding this comment

alexkirsz Mar 30, 2023

Choose a reason for hiding this comment

alexkirsz Mar 30, 2023

Choose a reason for hiding this comment

sokra Mar 30, 2023

Choose a reason for hiding this comment

alexkirsz Mar 31, 2023

Choose a reason for hiding this comment

sokra left a comment

Choose a reason for hiding this comment

alexkirsz commented Mar 30, 2023 •

edited

Loading

vercel bot commented Mar 30, 2023 •

edited

Loading

github-actions bot commented Mar 30, 2023 •

edited

Loading