fix: isolated chunk miss connection to chunksIdToUrlMap #988
Conversation
WalkthroughThis update focuses on enhancing the connectivity and optimization of chunk graphs within a codebase, ensuring consistency in message handling across JavaScript plugins, and improving code-splitting capabilities with the introduction of dynamic imports. The changes aim to streamline the merging and optimization of modules, connect dependencies more efficiently, and refine the user experience with more coherent and dynamic loading of components. Changes
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (6)
- crates/mako/src/chunk_graph.rs (1 hunks)
- crates/mako/src/optimize_chunk.rs (2 hunks)
- crates/node/src/js_plugin.rs (1 hunks)
- e2e/fixtures/code-splitting.complex/expect.js (1 hunks)
- e2e/fixtures/code-splitting.complex/src/should-be-merged.ts (1 hunks)
- e2e/fixtures/code-splitting.complex/src/vancant.ts (1 hunks)
Additional comments: 4
crates/node/src/js_plugin.rs (1)
- 37-37: The update to use
param.file.pathinstead ofparam.file.pathnamefor consistency is a good improvement.crates/mako/src/chunk_graph.rs (1)
- 195-216: The addition of the
connect_vacant_nodes_to_entry_chunkmethod is a well-implemented solution to ensure that vacant nodes are properly connected to the entry chunk, addressing the issue of disconnected nodes in the chunk graph.crates/mako/src/optimize_chunk.rs (2)
- 166-185: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [158-174]
The changes related to merging minimal async chunks into entry chunks and connecting optimized chunk dependents to the entry chunk enhance the optimization process and ensure the integrity of the chunk graph. These adjustments are well-implemented and address the optimization challenges effectively.
- 182-182: Retaining modules based on merged modules in async chunks is a necessary refinement to ensure that only relevant modules are included in the optimized chunks. This change improves the accuracy of the optimization process.
| import React from 'react'; | ||
| export default React.createElement("button", null, "inline") |
There was a problem hiding this comment.
The filename vancant.ts appears to be a typo. Consider renaming it to vacant.ts to avoid potential confusion or import errors.
| @@ -1,6 +1,7 @@ | |||
| import React from 'react'; | |||
| import context from './context'; | |||
|
|
|||
| console.log(React, context); | |||
| const vancant = React.lazy(() => import('./vancant')); | |||
There was a problem hiding this comment.
The component name vancant seems to be a typo. If the filename vancant.ts is corrected to vacant.ts, ensure to update this import to match: const vacant = React.lazy(() => import('./vacant'));.
- const vancant = React.lazy(() => import('./vancant'));
+ const vacant = React.lazy(() => import('./vacant'));Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const vancant = React.lazy(() => import('./vancant')); | |
| const vacant = React.lazy(() => import('./vacant')); |
| @@ -7,6 +7,10 @@ assert( | |||
| "minimal async chunk should be merged" | |||
| ); | |||
|
|
|||
| assert( | |||
| files["index.js"].includes("src_vancant_ts-async.js"), | |||
There was a problem hiding this comment.
If the filename and component name are corrected from vancant to vacant, ensure to update this assertion to match the new async chunk name: src_vacant_ts-async.js.
- files["index.js"].includes("src_vancant_ts-async.js"),
+ files["index.js"].includes("src_vacant_ts-async.js"),Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| files["index.js"].includes("src_vancant_ts-async.js"), | |
| files["index.js"].includes("src_vacant_ts-async.js"), |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- e2e/fixtures/code-splitting.complex/expect.js (1 hunks)
- e2e/fixtures/code-splitting.complex/src/should-be-merged.ts (1 hunks)
- e2e/fixtures/code-splitting.complex/src/vacant.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- e2e/fixtures/code-splitting.complex/expect.js
- e2e/fixtures/code-splitting.complex/src/should-be-merged.ts
Additional comments: 1
e2e/fixtures/code-splitting.complex/src/vacant.ts (1)
- 1-2: The changes in
vacant.tsintroduce a simple React component. The code is clean and follows best practices for React component creation in TypeScript.However, given the PR's focus on chunk graph optimization and ensuring proper node connections, please ensure this new test fixture is correctly integrated into the test suite and effectively validates the intended improvements.
Jinbao1001
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (5)
- crates/mako/src/chunk_graph.rs (1 hunks)
- crates/mako/src/optimize_chunk.rs (2 hunks)
- e2e/fixtures/code-splitting.complex/expect.js (1 hunks)
- e2e/fixtures/code-splitting.complex/src/isolated.ts (1 hunks)
- e2e/fixtures/code-splitting.complex/src/should-be-merged.ts (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- crates/mako/src/chunk_graph.rs
- crates/mako/src/optimize_chunk.rs
- e2e/fixtures/code-splitting.complex/expect.js
- e2e/fixtures/code-splitting.complex/src/should-be-merged.ts
Additional Context Used
Additional comments not posted (1)
e2e/fixtures/code-splitting.complex/src/isolated.ts (1)
1-2: The changes inisolated.tsintroduce a simple React component, likely intended as a test fixture for validating the chunk optimization process improvements. The code is syntactically correct and follows React best practices for component creation. Ensure this new component is adequately covered in the test scenarios related to chunk optimization and code splitting.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (2)
- crates/mako/src/chunk_graph.rs (1 hunks)
- crates/mako/src/optimize_chunk.rs (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- crates/mako/src/chunk_graph.rs
- crates/mako/src/optimize_chunk.rs
Additional Context Used
Jinbao1001
changed the title
optimize chunk过程, chunkgraph移除graph node时, 中会生成悬浮节点
如 (0, 16) (16, 22) 删除16节点, 两条边会同步移除
节点22成为悬浮节点, 导致深度遍历时无法建立依赖关系, chunksIdToUrlMap中文件映射缺失
解决方案: 在optimize结束后遍历出悬浮节点, 建立与跟节点的edge
Summary by CodeRabbit
New Features
Bug Fixes