Conversation
…or externals sharing the same section When multiple externals had different libraryName values but pointed to the same bundle URL and sectionPath, createLoadExternalSync/Async was invoked once per external, causing lynx.loadScript to execute redundantly for the same section. Add a sectionLoadTracker map keyed by (urlKey||sectionPath) so only the first external in each group triggers the load; subsequent externals in the group are assigned the already-loaded result directly.
🦋 Changeset detectedLatest commit: ad76375 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDeduplicates externals load logic so externals resolving to the same (bundle URL, section path, async) key emit a single Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 887397b6c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/webpack/externals-loading-webpack-plugin/test/cases/externals-loading/merge-loadscript-calls/index.js (1)
7-42: Please add a mixed async/sync regression variant for shared(url, sectionPath).This test is strong for sync dedup. A second case with shared pair but different
asyncvalues would guard against cross-mode reuse regressions in generated externals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webpack/externals-loading-webpack-plugin/test/cases/externals-loading/merge-loadscript-calls/index.js` around lines 7 - 42, Add a new test variant that covers the mixed async/sync case where two externals share the same (url, sectionPath) but differ in async mode so generated calls are not reused across modes: duplicate the existing test logic (using background and mainThread reads and the h0/h1 markers for createLoadExternalSync) but create one handler that is synchronous and another that is asynchronous (so the generator will emit both createLoadExternalSync and createLoadExternal or the async equivalent), then assert that each (bundle,section,mode) pair produces exactly one loader call (use split checks like background.split(hX).length / mainThread.split(hX).length for both sync and async variants or the appropriate marker strings), and also verify there is no cross-mode assignment reuse (similar to pkgBAssignment check) so PkgB does not incorrectly reuse PkgA across async vs sync paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/webpack/externals-loading-webpack-plugin/src/index.ts`:
- Around line 487-495: The deduplication key built as sectionKey =
`${urlKey}||${layerOptions.sectionPath}` can incorrectly reuse a mount value
across async vs sync externals; update the key to include the external's
async/type mode (e.g., append the external type or a boolean like isAsync) so
that sectionLoadTracker distinguishes entries by (urlKey, sectionPath,
externalMode). Locate the block using sectionKey, sectionLoadTracker,
existingMountVar, and mountVar and change the key construction and any places
that set/get from sectionLoadTracker so async and sync externals are never
conflated.
---
Nitpick comments:
In
`@packages/webpack/externals-loading-webpack-plugin/test/cases/externals-loading/merge-loadscript-calls/index.js`:
- Around line 7-42: Add a new test variant that covers the mixed async/sync case
where two externals share the same (url, sectionPath) but differ in async mode
so generated calls are not reused across modes: duplicate the existing test
logic (using background and mainThread reads and the h0/h1 markers for
createLoadExternalSync) but create one handler that is synchronous and another
that is asynchronous (so the generator will emit both createLoadExternalSync and
createLoadExternal or the async equivalent), then assert that each
(bundle,section,mode) pair produces exactly one loader call (use split checks
like background.split(hX).length / mainThread.split(hX).length for both sync and
async variants or the appropriate marker strings), and also verify there is no
cross-mode assignment reuse (similar to pkgBAssignment check) so PkgB does not
incorrectly reuse PkgA across async vs sync paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 39accffa-b80e-40a7-bbd3-ae5d86979f3b
📒 Files selected for processing (5)
.changeset/fix-duplicate-loadscript-calls.mdpackages/webpack/externals-loading-webpack-plugin/src/index.tspackages/webpack/externals-loading-webpack-plugin/test/cases/externals-loading/merge-loadscript-calls/index.jspackages/webpack/externals-loading-webpack-plugin/test/cases/externals-loading/merge-loadscript-calls/rspack.config.jspackages/webpack/externals-loading-webpack-plugin/test/cases/externals-loading/merge-loadscript-calls/test.config.cjs
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…tion dedup - Include `async` in the sectionKey so sync and async externals sharing the same section are not merged (they resolve to different runtime shapes: plain value vs Promise) - Restore the `=== undefined` guard on the alias assignment so host-injected globals are not unconditionally overwritten
Merging this PR will degrade performance by 10.31%
Performance Changes
Comparing 
Footnotes
|
React Example#7310 Bundle Size — 223.58KiB (0%).ad76375(current) vs d1c16c5 main#7300(baseline) Bundle metrics
|
| Current #7310 |
Baseline #7300 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
179 |
179 |
| Duplicate Modules |
70 |
70 |
| Duplicate Code |
45.76% |
45.76% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #7310 |
Baseline #7300 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
77.82KiB |
77.82KiB |
Bundle analysis report Branch fix/deduplicate-loadscript-calls Project dashboard
Generated by RelativeCI Documentation Report issue
React External#427 Bundle Size — 582.81KiB (0%).ad76375(current) vs d1c16c5 main#418(baseline) Bundle metrics
|
| Current #427 |
Baseline #418 |
|
|---|---|---|
| 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 fix/deduplicate-loadscript-calls Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#8884 Bundle Size — 749.3KiB (0%).ad76375(current) vs d1c16c5 main#8874(baseline) Bundle metrics
|
| Current #8884 |
Baseline #8874 |
|
|---|---|---|
| Initial JS |
44.45KiB |
44.45KiB |
| Initial CSS |
2.16KiB |
2.16KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
8 |
8 |
| Assets |
10 |
10 |
 Modules |
148(-0.67%) |
149 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
35.02%(+0.03%) |
35.01% |
| Packages |
3 |
3 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #8884 |
Baseline #8874 |
|
|---|---|---|
| Other |
401.63KiB |
401.63KiB |
| JS |
345.51KiB |
345.51KiB |
| CSS |
2.16KiB |
2.16KiB |
Bundle analysis report Branch fix/deduplicate-loadscript-calls Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#443 Bundle Size — 192.85KiB (0%).ad76375(current) vs d1c16c5 main#433(baseline) Bundle metrics
|
| Current #443 |
Baseline #433 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
173 |
173 |
| Duplicate Modules |
67 |
67 |
| Duplicate Code |
45.36% |
45.36% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #443 |
Baseline #433 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
81.61KiB |
81.61KiB |
Bundle analysis report Branch fix/deduplicate-loadscript-calls Project dashboard
Generated by RelativeCI Documentation Report issue
…ync isolation Extend merge-loadscript-calls with async externals: - pkg-d and pkg-e: two async externals sharing the same (bundle, section) — the async group should merge, calling createLoadExternalAsync only once. - pkg-f: a sync external sharing the SAME (bundle, section) as pkg-d/pkg-e — must NOT merge with the async group because the runtime shape differs (plain value vs Promise).
3 participants
Summary
When multiple externals had different
libraryNamevalues but pointed to the same bundle URL andsectionPath,createLoadExternalSync/createLoadExternalAsyncwas called once per external, causinglynx.loadScriptto execute redundantly for the same section.Before (example with multiple aliases sharing the same section):
After:
Fix: Added a
sectionLoadTrackermap (keyed byurlKey||sectionPath||async) in#genExternalsLoadingCode. The first external for each(bundle, section, async)triple generates the actualcreateLoadExternalSync/Asynccall; subsequent externals in the same group reuse the already-loaded result (still guarded by=== undefinedso host-injected globals are preserved).asyncis included in the key because sync and async externals resolve to different runtime shapes (plain value vs Promise) and must not be merged.Checklist
Summary by CodeRabbit