-
Notifications
You must be signed in to change notification settings - Fork 123
fix(externals-loading-webpack-plugin): deduplicate loadScript calls for externals sharing the same section #2465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
887397b
fix(externals-loading-webpack-plugin): deduplicate loadScript calls f…
upupming aa27ab5
fix(externals-loading-webpack-plugin): address review comments on sec…
upupming ad76375
test(externals-loading-webpack-plugin): cover async dedup and async/s…
upupming File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| --- | ||
| '@lynx-js/externals-loading-webpack-plugin': patch | ||
| --- | ||
|
|
||
| fix: deduplicate `loadScript` calls for externals sharing the same (bundle, section) pair | ||
|
|
||
| When multiple externals had different `libraryName` values but pointed to the same | ||
| bundle URL and section path, `createLoadExternalSync`/`createLoadExternalAsync` was | ||
| called once per external, causing `lynx.loadScript` to execute redundantly for the | ||
| same section. Now only the first external in each `(url, sectionPath)` group triggers | ||
| the load; subsequent externals in the group are assigned the already-loaded result | ||
| directly. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
66 changes: 66 additions & 0 deletions
66
...rnals-loading-webpack-plugin/test/cases/externals-loading/merge-loadscript-calls/index.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| import { a } from 'pkg-a'; | ||
| import { b } from 'pkg-b'; | ||
| import { c } from 'pkg-c'; | ||
| import { f } from 'pkg-f'; | ||
|
|
||
| const d = await import('pkg-d'); | ||
| const e = await import('pkg-e'); | ||
|
|
||
| console.info(a, b, c, d, e, f); | ||
|
|
||
| it('should call loadScript only once per (bundle, section) pair', async () => { | ||
| const fs = await import('node:fs'); | ||
| const path = await import('node:path'); | ||
|
|
||
| const background = fs.readFileSync( | ||
| path.resolve(__dirname, 'main:background.js'), | ||
| 'utf-8', | ||
| ); | ||
| const mainThread = fs.readFileSync( | ||
| path.resolve(__dirname, 'main:main-thread.js'), | ||
| 'utf-8', | ||
| ); | ||
|
|
||
| // Use concatenation to avoid the literal pattern appearing inside this compiled file itself. | ||
|
|
||
| // pkg-a and pkg-b share the same (url, sectionPath). Only ONE createLoadExternalSync call | ||
| // should be generated for that pair; pkg-b reuses pkg-a's result directly. | ||
| // pkg-c has a different bundle URL, so it gets its own call. | ||
| // => Exactly one call per handler (2 total), not one per external (3 total). | ||
|
|
||
| // Match actual invocations of the form `createLoadExternalSync/Async(handlerN, ...` | ||
| // The function definitions use `(handler,` (no digit), so they won't be counted here. | ||
| const syncH0 = 'createLoadExternalSync' + '(handler0,'; | ||
| const syncH1 = 'createLoadExternalSync' + '(handler1,'; | ||
| const syncH2 = 'createLoadExternalSync' + '(handler2,'; | ||
| const asyncH2 = 'createLoadExternalAsync' + '(handler2,'; | ||
|
|
||
| // Sync-side: handler0 and handler1 each get exactly one Sync call (PkgB is merged into PkgA). | ||
| expect(background.split(syncH0).length).toBe(2); | ||
| expect(background.split(syncH1).length).toBe(2); | ||
| expect(mainThread.split(syncH0).length).toBe(2); | ||
| expect(mainThread.split(syncH1).length).toBe(2); | ||
|
|
||
| // handler2 is shared by async pkg-d/pkg-e and sync pkg-f. The async group merges | ||
| // (PkgE reuses PkgD), so exactly one Async call. The sync pkg-f must NOT merge | ||
| // with the async group (different runtime shape), so exactly one Sync call on handler2. | ||
| expect(background.split(asyncH2).length).toBe(2); | ||
| expect(background.split(syncH2).length).toBe(2); | ||
| expect(mainThread.split(asyncH2).length).toBe(2); | ||
| expect(mainThread.split(syncH2).length).toBe(2); | ||
|
|
||
| // PkgB reuses PkgA's already-loaded global — no createLoadExternal call. | ||
| // The === undefined guard is preserved so host-injected values are not overwritten. | ||
| const pkgBAssignment = '["PkgB"] === undefined ? ' | ||
| + 'lynx[Symbol.for(\'__LYNX_EXTERNAL_GLOBAL__\')]["PkgA"] : ' | ||
| + 'lynx[Symbol.for(\'__LYNX_EXTERNAL_GLOBAL__\')]["PkgB"]'; | ||
| expect(background).toContain(pkgBAssignment); | ||
| expect(mainThread).toContain(pkgBAssignment); | ||
|
|
||
| // PkgE reuses PkgD (both async, same section) — no extra createLoadExternalAsync call. | ||
| const pkgEAssignment = '["PkgE"] === undefined ? ' | ||
| + 'lynx[Symbol.for(\'__LYNX_EXTERNAL_GLOBAL__\')]["PkgD"] : ' | ||
| + 'lynx[Symbol.for(\'__LYNX_EXTERNAL_GLOBAL__\')]["PkgE"]'; | ||
| expect(background).toContain(pkgEAssignment); | ||
| expect(mainThread).toContain(pkgEAssignment); | ||
| }); |
89 changes: 89 additions & 0 deletions
89
...ading-webpack-plugin/test/cases/externals-loading/merge-loadscript-calls/rspack.config.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| import { createConfig } from '../../../helpers/create-config.js'; | ||
|
|
||
| /** @type {import('@rspack/core').Configuration} */ | ||
| export default { | ||
| context: __dirname, | ||
| ...createConfig( | ||
| { | ||
| backgroundLayer: 'background', | ||
| mainThreadLayer: 'main-thread', | ||
| externals: { | ||
| // Two different library names pointing to the same (url, sectionPath). | ||
| // Only one loadScript call should be generated per section. | ||
| 'pkg-a': { | ||
| libraryName: 'PkgA', | ||
| url: 'https://example.com/common.bundle', | ||
| async: false, | ||
| background: { | ||
| sectionPath: 'common', | ||
| }, | ||
| mainThread: { | ||
| sectionPath: 'common__main-thread', | ||
| }, | ||
| }, | ||
| 'pkg-b': { | ||
| libraryName: 'PkgB', | ||
| url: 'https://example.com/common.bundle', | ||
| async: false, | ||
| background: { | ||
| sectionPath: 'common', | ||
| }, | ||
| mainThread: { | ||
| sectionPath: 'common__main-thread', | ||
| }, | ||
| }, | ||
| // A third external with the same section but different bundle — should still | ||
| // generate its own loadScript call. | ||
| 'pkg-c': { | ||
| libraryName: 'PkgC', | ||
| url: 'https://example.com/other.bundle', | ||
| async: false, | ||
| background: { | ||
| sectionPath: 'common', | ||
| }, | ||
| mainThread: { | ||
| sectionPath: 'common__main-thread', | ||
| }, | ||
| }, | ||
| // Two async externals sharing the same (url, sectionPath). They should merge | ||
| // via createLoadExternalAsync. | ||
| 'pkg-d': { | ||
| libraryName: 'PkgD', | ||
| url: 'https://example.com/async.bundle', | ||
| async: true, | ||
| background: { | ||
| sectionPath: 'shared', | ||
| }, | ||
| mainThread: { | ||
| sectionPath: 'shared__main-thread', | ||
| }, | ||
| }, | ||
| 'pkg-e': { | ||
| libraryName: 'PkgE', | ||
| url: 'https://example.com/async.bundle', | ||
| async: true, | ||
| background: { | ||
| sectionPath: 'shared', | ||
| }, | ||
| mainThread: { | ||
| sectionPath: 'shared__main-thread', | ||
| }, | ||
| }, | ||
| // A sync external sharing the SAME (url, sectionPath) as the async ones above. | ||
| // Must NOT be merged with the async group because the runtime shape differs | ||
| // (plain value vs Promise). | ||
| 'pkg-f': { | ||
| libraryName: 'PkgF', | ||
| url: 'https://example.com/async.bundle', | ||
| async: false, | ||
| background: { | ||
| sectionPath: 'shared', | ||
| }, | ||
| mainThread: { | ||
| sectionPath: 'shared__main-thread', | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| ), | ||
| }; |
7 changes: 7 additions & 0 deletions
7
...oading-webpack-plugin/test/cases/externals-loading/merge-loadscript-calls/test.config.cjs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| /** @type {import("@lynx-js/test-tools").TConfigCaseConfig} */ | ||
| module.exports = { | ||
| bundlePath: [ | ||
| 'main:main-thread.js', | ||
| 'main:background.js', | ||
| ], | ||
| }; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.