feat(chunk-loading): bypass Promise.all for single sync-then chunk loads#2597
Conversation
🦋 Changeset detectedLatest commit: 5abf278 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughA patch release adds a ChangesSync-then chunk load optimization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/webpack/chunk-loading-webpack-plugin/test/cases/chunk-loading/ensure-chunk-shortcut/index.js (1)
28-32: 💤 Low valueConsider wrapping require() in error handling.
The
loadLazyBundlemock callsrequire()directly on line 30, which will throw synchronously if the bundle file is missing or fails to parse. While this is acceptable in test code (failing fast when fixtures are broken), wrapping it in a try-catch and returning a rejected promise would make the mock more robust and consistent with typical async error handling.♻️ Optional: Add error handling
loadLazyBundle: rstest.fn(function loadLazyBundle(request) { + try { - return makeSyncThenPromise( - require(path.join(__dirname, `${request}.rspack.bundle.cjs`)), - ); + return makeSyncThenPromise( + require(path.join(__dirname, `${request}.rspack.bundle.cjs`)), + ); + } catch (error) { + return Promise.reject(error); + } }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/webpack/chunk-loading-webpack-plugin/test/cases/chunk-loading/ensure-chunk-shortcut/index.js` around lines 28 - 32, The mock loadLazyBundle function currently calls require(path.join(__dirname, `${request}.rspack.bundle.cjs`)) synchronously which will throw on missing or invalid bundles; modify the loadLazyBundle (the rstest.fn named loadLazyBundle) to wrap the require call in a try-catch and, on error, return a rejected promise (or pass the error into makeSyncThenPromise as a rejection) so consumers receive an async rejection instead of an uncaught synchronous throw.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/webpack/chunk-loading-webpack-plugin/test/cases/chunk-loading/ensure-chunk-shortcut/index.js`:
- Around line 28-32: The mock loadLazyBundle function currently calls
require(path.join(__dirname, `${request}.rspack.bundle.cjs`)) synchronously
which will throw on missing or invalid bundles; modify the loadLazyBundle (the
rstest.fn named loadLazyBundle) to wrap the require call in a try-catch and, on
error, return a rejected promise (or pass the error into makeSyncThenPromise as
a rejection) so consumers receive an async rejection instead of an uncaught
synchronous throw.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dcf4e2a0-00ea-4035-bf7e-b6efe78917ae
📒 Files selected for processing (3)
packages/webpack/chunk-loading-webpack-plugin/test/cases/chunk-loading/ensure-chunk-shortcut/dynamic.jspackages/webpack/chunk-loading-webpack-plugin/test/cases/chunk-loading/ensure-chunk-shortcut/index.jspackages/webpack/chunk-loading-webpack-plugin/test/cases/chunk-loading/ensure-chunk-shortcut/rspack.config.js
✅ Files skipped from review due to trivial changes (1)
- packages/webpack/chunk-loading-webpack-plugin/test/cases/chunk-loading/ensure-chunk-shortcut/dynamic.js
`__webpack_require__.e` always wrapped chunk handlers in `Promise.all`, which collapses any sync-then promise back into a native Promise. As a result, preact's `lazy(loader)` always saw a pending `prom` at first render and fell through to Suspense fallback, even when the underlying `lynx.loadLazyBundle` resolved synchronously (cached / fetchBundle wait). Override `e` from the Lynx chunk-loading runtime: when exactly one handler pushed exactly one promise into the array — the dominant lazy-bundle shape — return that promise directly so the sync-then contract survives all the way to preact. Length 0 and length > 1 fall through to the original Promise.all path.
Use the same Object.keys(...).reduce(...) pattern that rspack's default __webpack_require__.e emits, so the overridden version differs only in the final Promise.all-vs-shortcut decision.
Mirror the existing runtime-globals case shape: mock lynx.loadLazyBundle so it returns a promise carrying a sync-then marker, then assert that __webpack_require__.e preserves the marker on a single-promise load and strips it (falling back to native Promise.all) when multiple handlers push promises.
upupming
force-pushed
the
feat/sync-then-chunk-load
branch
from
5e7066c to
5abf278
Compare
Merging this PR will degrade performance by 16.18%
Performance Changes
Comparing 
Footnotes
|
React External#1119 Bundle Size — 690.27KiB (0%).5abf278(current) vs d588d03 main#1112(baseline) Bundle metrics
|
| Current #1119 |
Baseline #1112 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
17 |
17 |
| Duplicate Modules |
5 |
5 |
| Duplicate Code |
8.59% |
8.59% |
| Packages |
0 |
0 |
| Duplicate Packages |
0 |
0 |
Bundle analysis report Branch feat/sync-then-chunk-load Project dashboard
Generated by RelativeCI Documentation Report issue
React Example with Element Template#270 Bundle Size — 197.79KiB (0%).5abf278(current) vs d588d03 main#263(baseline) Bundle metrics
|
| Current #270 |
Baseline #263 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
 Modules |
81(+2.53%) |
79 |
| Duplicate Modules |
23 |
23 |
| Duplicate Code |
40.29%(-0.1%) |
40.33% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #270 |
Baseline #263 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
52.03KiB |
52.03KiB |
Bundle analysis report Branch feat/sync-then-chunk-load Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#1135 Bundle Size — 206.6KiB (0%).5abf278(current) vs d588d03 main#1128(baseline) Bundle metrics
|
| Current #1135 |
Baseline #1128 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
192 |
192 |
| Duplicate Modules |
77 |
77 |
| Duplicate Code |
44.36% |
44.36% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #1135 |
Baseline #1128 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
95.37KiB |
95.37KiB |
Bundle analysis report Branch feat/sync-then-chunk-load Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#9577 Bundle Size — 900.04KiB (0%).5abf278(current) vs d588d03 main#9570(baseline) Bundle metrics
|
| Current #9577 |
Baseline #9570 |
|
|---|---|---|
| Initial JS |
44.46KiB |
44.46KiB |
| Initial CSS |
2.22KiB |
2.22KiB |
| Cache Invalidation |
0% |
13.59% |
| Chunks |
9 |
9 |
| Assets |
11 |
11 |
| Modules |
227(-0.87%) |
229 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
27.28% |
27.28% |
| Packages |
10 |
10 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #9577 |
Baseline #9570 |
|
|---|---|---|
| JS |
495.91KiB |
495.91KiB |
| Other |
401.92KiB |
401.92KiB |
| CSS |
2.22KiB |
2.22KiB |
Bundle analysis report Branch feat/sync-then-chunk-load Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#8005 Bundle Size — 235.77KiB (0%).5abf278(current) vs d588d03 main#7998(baseline) Bundle metrics
|
| Current #8005 |
Baseline #7998 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
197 |
197 |
| Duplicate Modules |
80 |
80 |
| Duplicate Code |
44.85% |
44.85% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #8005 |
Baseline #7998 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
90.01KiB |
90.01KiB |
Bundle analysis report Branch feat/sync-then-chunk-load Project dashboard
Generated by RelativeCI Documentation Report issue
2 participants
Summary
__webpack_require__.ealways wrapped chunk handlers' promises inPromise.all, which collapses any sync-then promise back into a native one. preact'slazy(loader)therefore always saw a pending `prom` at first render and fell through to the Suspense fallback — even when the underlying `lynx.loadLazyBundle` resolved synchronously (cached / `fetchBundle.wait()`).Override `e` from the Lynx chunk-loading runtime so that when exactly one handler pushes exactly one promise (the typical lazy-bundle shape — one JS chunk, no separate CSS chunk, no shared splits), we return that promise directly. The sync-then contract survives all the way to preact and the lazy component renders on the first pass.
Length 0 and length > 1 fall through to the original `Promise.all` path; behavior for those is unchanged.
Test plan
Summary by CodeRabbit
Bug Fixes
Tests
Chores