feat: support bundle and load css in external bundle#2143
feat: support bundle and load css in external bundle#2143
Conversation
🦋 Changeset detectedLatest commit: a80ede6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
|
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:
📝 WalkthroughWalkthroughAdds CSS extraction, serialization, and runtime loading for external bundles: encoder emits CSS custom-sections, React plugin loader behavior updated, externals runtime loads/adopts styles on main-thread, and deps/tests/examples updated to cover CSS in external bundles. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Merging this PR will improve performance by 7.69%
Performance Changes
Comparing Footnotes
|
Web Explorer#7943 Bundle Size — 383.53KiB (0%).a80ede6(current) vs 6cbe5d8 main#7929(baseline) Bundle metrics
Bundle size by type
|
| Current #7943 |
Baseline #7929 |
|
|---|---|---|
252.58KiB |
252.58KiB |
|
95.85KiB |
95.85KiB |
|
35.1KiB |
35.1KiB |
Bundle analysis report Branch feat/rslib-css Project dashboard
Generated by RelativeCI Documentation Report issue
8e47037 to
d9c0752
Compare
af8e5a2 to
872c04a
Compare
20b24a0 to
39c3de2
Compare
…r` (#2269) <!-- Thank you for submitting a pull request! We appreciate the time and effort you have invested in making these changes. Please ensure that you provide enough information to allow others to review your pull request. Upon submission, your pull request will be automatically assigned with reviewers. If you want to learn more about contributing to this project, please visit: https://github.com/lynx-family/lynx-stack/blob/main/CONTRIBUTING.md. --> <!-- The AI summary below will be auto-generated - feel free to replace it with your own. --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Patched @lynx-js/template-webpack-plugin and @lynx-js/css-serializer. * **New Features** * Exposed CSS namespace directly from @lynx-js/css-serializer. * Added CSSPlugins export with parserPlugins property. * Made cssChunksToMap available as a public export from @lynx-js/css-serializer. * **Chores** * Updated test imports and configuration for improved module resolution. <!-- end of auto-generated comment: release notes by coderabbit.ai --> In #2143, we need to use `cssChunksToMap` in `@lynx-js/lynx-bundle-rslib-config` package. To avoid repeating ourselves, we extract the `cssChunksToMap` API from `@lynx-js/template-webpack-plugin` to `@lynx-js/css-serializer`. ## Checklist <!--- Check and mark with an "x" --> - [x] Tests updated (or not required). - [ ] Documentation updated (or not required). - [x] Changeset added, and when a BREAKING CHANGE occurs, it needs to be clearly marked (or not required).
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/webpack/template-webpack-plugin/etc/template-webpack-plugin.api.md (1)
19-24: No direct edit needed in generated API report.This warning is a downstream artifact from
packages/webpack/template-webpack-plugin/src/index.ts; fixing the source declaration is the right path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webpack/template-webpack-plugin/etc/template-webpack-plugin.api.md` around lines 19 - 24, The API report flags CSSPlugins as missing a release tag; update the source export for CSSPlugins in the module where it's declared by adding a JSDoc release tag (e.g., /** `@public` */) above the export so the generated declaration includes the tag, and ensure the related Plugins type used in the CSSPlugins.parserPlugins typing is correctly exported/imported (symbols: CSSPlugins, Plugins) so the API extractor can resolve types.
🧹 Nitpick comments (1)
packages/rspeedy/lynx-bundle-rslib-config/src/webpack/ExternalBundleWebpackPlugin.ts (1)
118-153: Prefer mutating the reducer accumulator to avoid repeated object cloning.Line 118-Line 153 rebuilds
prevwith object spread on every asset. This creates avoidable O(n²)-style allocation churn for larger builds.♻️ Suggested refactor
- const customSections = assets - .reduce<Record<string, { content: string | { ruleList: LynxStyleNode[] } }>>( - (prev, cur) => { - switch (cur.info['assetType']) { - case 'javascript': - return ({ - ...prev, - [cur.name.replace(/\.js$/, '')]: { - content: cur.source.source().toString(), - }, - }) - case 'extract-css': - return ({ - ...prev, - [`${cur.name.replace(/\.css$/, '')}:CSS`]: { - 'encoding': 'CSS', - content: { - ruleList: cssChunksToMap( - [cur.source.source().toString()], - [], - true, - ).cssMap[0] ?? [], - }, - }, - }) - default: - return prev - } - }, - {}, - ) + const customSections: Record<string, { content: string | { ruleList: LynxStyleNode[] } }> = {} + for (const cur of assets) { + switch (cur.info['assetType']) { + case 'javascript': + customSections[cur.name.replace(/\.js$/, '')] = { + content: cur.source.source().toString(), + } + break + case 'extract-css': + customSections[`${cur.name.replace(/\.css$/, '')}:CSS`] = { + encoding: 'CSS', + content: { + ruleList: cssChunksToMap([cur.source.source().toString()], [], true).cssMap[0] ?? [], + }, + } as never + break + default: + break + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rspeedy/lynx-bundle-rslib-config/src/webpack/ExternalBundleWebpackPlugin.ts` around lines 118 - 153, The reducer currently rebuilds the accumulator with object spread on each iteration causing O(n²) allocations; in the reduce callback (the function passed to .reduce in ExternalBundleWebpackPlugin) stop returning new objects and instead mutate the accumulator `prev`: for 'javascript' set prev[cur.name.replace(/\.js$/, '')] = { content: cur.source.source().toString() } and for 'extract-css' set prev[`${cur.name.replace(/\.css$/, '')}:CSS`] = { encoding: 'CSS', content: { ruleList: cssChunksToMap([cur.source.source().toString()], [], true).cssMap[0] ?? [] } }; then return prev; keep the reduce generic/typing for Record<string, { content: string | { ruleList: LynxStyleNode[] } }> unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/react-externals/src/index.css`:
- Line 8: The empty CSS rule `.dummy {}` is failing Stylelint's
`block-no-empty`; update the `.dummy` rule to be non-empty (e.g., add a harmless
declaration like `display: block;` or a comment/declaration that preserves the
trigger behavior) so the selector remains but the block no longer violates
`block-no-empty`; locate the `.dummy` rule in the CSS and add the single
non-empty declaration or comment inside it.
In `@packages/rspeedy/lynx-bundle-rslib-config/test/external-bundle.test.ts`:
- Around line 172-201: The test sets an environment stub via vi.stubEnv('DEBUG',
'rsbuild,rslib,rspack') but never restores it, causing cross-test leakage;
change the call to capture the returned restore function (e.g., const
restoreDebug = vi.stubEnv(...)) and call restoreDebug() after the
build/decode/assertions (or in a finally block) so the DEBUG env is reset for
subsequent tests; reference vi.stubEnv in the test around the
build(rslibConfig)/decodeTemplate(...)/assert expectations to locate where to
add the restore call.
In `@packages/webpack/externals-loading-webpack-plugin/src/index.ts`:
- Line 330: Replace the hard-coded '__main-thread' suffix used when deriving the
CSS section name with the configured mainThreadLayer value: instead of
sectionPath.replace('__main-thread', '') + ':CSS' use
sectionPath.replace(mainThreadLayer, '') + ':CSS' (apply the same change in both
the async and sync branches where __LoadStyleSheet is called), so that
__LoadStyleSheet(sectionPath.replace(mainThreadLayer, '') + ':CSS',
response.url) correctly resolves customized main thread layer names.
- Line 340: The catch path builds an Error message using error.message which can
throw if a non-Error (null/primitive) was thrown; replace the direct access with
a guarded conversion: derive a safeMessage by checking if error is an object
with a message property and using that, otherwise use String(error) (or a
default string), then construct the new Error with that safeMessage and set the
cause only when appropriate (or pass the original thrown value as cause if
supported). Apply the same guarded logic to both occurrences where reject(new
Error('Failed to load script ' + sectionPath + ' in ' + response.url + ': ' +
error.message, { cause: error })) is used so non-Error throws are handled
safely.
---
Duplicate comments:
In `@packages/webpack/template-webpack-plugin/etc/template-webpack-plugin.api.md`:
- Around line 19-24: The API report flags CSSPlugins as missing a release tag;
update the source export for CSSPlugins in the module where it's declared by
adding a JSDoc release tag (e.g., /** `@public` */) above the export so the
generated declaration includes the tag, and ensure the related Plugins type used
in the CSSPlugins.parserPlugins typing is correctly exported/imported (symbols:
CSSPlugins, Plugins) so the API extractor can resolve types.
---
Nitpick comments:
In
`@packages/rspeedy/lynx-bundle-rslib-config/src/webpack/ExternalBundleWebpackPlugin.ts`:
- Around line 118-153: The reducer currently rebuilds the accumulator with
object spread on each iteration causing O(n²) allocations; in the reduce
callback (the function passed to .reduce in ExternalBundleWebpackPlugin) stop
returning new objects and instead mutate the accumulator `prev`: for
'javascript' set prev[cur.name.replace(/\.js$/, '')] = { content:
cur.source.source().toString() } and for 'extract-css' set
prev[`${cur.name.replace(/\.css$/, '')}:CSS`] = { encoding: 'CSS', content: {
ruleList: cssChunksToMap([cur.source.source().toString()], [], true).cssMap[0]
?? [] } }; then return prev; keep the reduce generic/typing for Record<string, {
content: string | { ruleList: LynxStyleNode[] } }> unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
.changeset/thick-needles-rest.md.changeset/wet-rockets-taste.mdexamples/react-externals/src/index.cssexamples/react-externals/src/index.tsxpackages/rspeedy/lynx-bundle-rslib-config/package.jsonpackages/rspeedy/lynx-bundle-rslib-config/src/webpack/ExternalBundleWebpackPlugin.tspackages/rspeedy/lynx-bundle-rslib-config/test/external-bundle.test.tspackages/rspeedy/lynx-bundle-rslib-config/test/fixtures/css-lib/index.csspackages/rspeedy/lynx-bundle-rslib-config/test/fixtures/css-lib/index.tspackages/rspeedy/plugin-react/src/css.tspackages/rspeedy/plugin-react/src/pluginReactLynx.tspackages/tools/css-serializer/src/css/ast.tspackages/tools/css-serializer/src/css/cssChunksToMap.tspackages/tools/css-serializer/src/css/debundle.tspackages/tools/css-serializer/src/css/index.tspackages/tools/css-serializer/src/index.tspackages/tools/css-serializer/test/css.test.tspackages/tools/css-serializer/vitest.config.tspackages/webpack/externals-loading-webpack-plugin/src/index.tspackages/webpack/externals-loading-webpack-plugin/test/helpers/setup-env.jspackages/webpack/template-webpack-plugin/etc/template-webpack-plugin.api.mdpackages/webpack/template-webpack-plugin/package.jsonpackages/webpack/template-webpack-plugin/src/LynxTemplatePlugin.tspackages/webpack/template-webpack-plugin/src/WebEncodePlugin.tspackages/webpack/template-webpack-plugin/src/css/index.tspackages/webpack/template-webpack-plugin/src/index.tspackages/webpack/template-webpack-plugin/src/web/genStyleInfo.ts
💤 Files with no reviewable changes (1)
- packages/webpack/template-webpack-plugin/src/css/index.ts
packages/rspeedy/lynx-bundle-rslib-config/test/external-bundle.test.ts
Outdated
Show resolved
Hide resolved
…nx-family/lynx-stack into feat/rslib-css
ee8f034 to
1c41029
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/webpack/externals-loading-webpack-plugin/src/index.ts (2)
341-341:⚠️ Potential issue | 🟡 MinorHarden catch-path message construction for non-
Errorthrows.Line 341 and Line 368 assume
error.messageexists; primitives/null can cause a second failure in error handling.🛡️ Suggested fix
- reject(new Error('Failed to load script ' + sectionPath + ' in ' + response.url + ': ' + error.message, { cause: error })) + const reason = (error && typeof error === 'object' && 'message' in error) + ? String(error.message) + : String(error); + reject(new Error('Failed to load script ' + sectionPath + ' in ' + response.url + ': ' + reason, { cause: error }))- throw new Error('Failed to load script ' + sectionPath + ' in ' + response.url + ': ' + error.message, { cause: error }) + const reason = (error && typeof error === 'object' && 'message' in error) + ? String(error.message) + : String(error); + throw new Error('Failed to load script ' + sectionPath + ' in ' + response.url + ': ' + reason, { cause: error })Also applies to: 368-368
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webpack/externals-loading-webpack-plugin/src/index.ts` at line 341, The catch-path builds a new Error using error.message which can throw if the thrown value is null/primitive; replace uses of error.message in the reject(new Error(..., { cause: error })) expression with a safe stringifier (e.g. const safeMsg = (error && typeof error === 'object' && 'message' in error) ? (error as Error).message : String(error)) and use safeMsg in the constructed message, and optionally only pass cause when error is an Error instance (e.g. cause: error instanceof Error ? error : undefined) to harden both occurrences that currently reference error.message.
330-331:⚠️ Potential issue | 🟠 MajorUse configured main-thread suffix for CSS section derivation.
Line 330 and Line 357 still hard-code
__main-thread, so CSS lookup can fail whenmainThreadLayeris customized.🔧 Suggested fix
- const styleSheet = __LoadStyleSheet(sectionPath.replace('__main-thread', '') + ':CSS', response.url); + const mainThreadSuffix = '__${externalsLoadingPluginOptions.mainThreadLayer}'; + const cssSectionPath = sectionPath.endsWith(mainThreadSuffix) + ? sectionPath.slice(0, -mainThreadSuffix.length) + : sectionPath; + const styleSheet = __LoadStyleSheet(cssSectionPath + ':CSS', response.url);Also applies to: 357-358
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webpack/externals-loading-webpack-plugin/src/index.ts` around lines 330 - 331, Replace the hard-coded "__main-thread" string with the configured main-thread suffix (mainThreadLayer) when deriving the CSS section path: change the calls that compute the CSS lookup (the sectionPath.replace('__main-thread', '') + ':CSS' used before passing to __LoadStyleSheet) to remove the configured mainThreadLayer instead of the literal, and apply the same change to the other occurrence that currently uses "__main-thread"; ensure you remove only the suffix instance of mainThreadLayer from sectionPath (preserving other parts) before appending ":CSS" and calling __LoadStyleSheet(response.url).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/webpack/externals-loading-webpack-plugin/src/index.ts`:
- Line 341: The catch-path builds a new Error using error.message which can
throw if the thrown value is null/primitive; replace uses of error.message in
the reject(new Error(..., { cause: error })) expression with a safe stringifier
(e.g. const safeMsg = (error && typeof error === 'object' && 'message' in error)
? (error as Error).message : String(error)) and use safeMsg in the constructed
message, and optionally only pass cause when error is an Error instance (e.g.
cause: error instanceof Error ? error : undefined) to harden both occurrences
that currently reference error.message.
- Around line 330-331: Replace the hard-coded "__main-thread" string with the
configured main-thread suffix (mainThreadLayer) when deriving the CSS section
path: change the calls that compute the CSS lookup (the
sectionPath.replace('__main-thread', '') + ':CSS' used before passing to
__LoadStyleSheet) to remove the configured mainThreadLayer instead of the
literal, and apply the same change to the other occurrence that currently uses
"__main-thread"; ensure you remove only the suffix instance of mainThreadLayer
from sectionPath (preserving other parts) before appending ":CSS" and calling
__LoadStyleSheet(response.url).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/rspeedy/lynx-bundle-rslib-config/test/external-bundle.test.tspackages/webpack/externals-loading-webpack-plugin/src/index.tspackages/webpack/template-webpack-plugin/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/webpack/template-webpack-plugin/package.json
| "@lynx-js/runtime-wrapper-webpack-plugin": "workspace:*", | ||
| "@lynx-js/tasm": "0.0.20" | ||
| "@lynx-js/tasm": "0.0.26", | ||
| "css-tree": "^3.1.0" |
There was a problem hiding this comment.
It seems like the css-tree package has never been used.
| function removeIgnoreCSS(rule: ReturnType<typeof chain.module.rule>) { | ||
| if ( | ||
| // Rslib has a builtin ignore-css-loader | ||
| // We need to remove it and use our own ignore-css-loader | ||
| rule.uses.has(CHAIN_ID.USE.IGNORE_CSS) | ||
| ) { | ||
| rule.uses.delete(CHAIN_ID.USE.IGNORE_CSS) | ||
| } | ||
| } | ||
|
|
||
| if (!chain.plugins.has(CHAIN_ID.PLUGIN.MINI_CSS_EXTRACT)) { | ||
| chain | ||
| .plugin(CHAIN_ID.PLUGIN.MINI_CSS_EXTRACT) | ||
| .use(CssExtractPlugin) | ||
| } |
There was a problem hiding this comment.
This should be related with output.emitCss option, which defaults to true when output.target === 'web'.
Instead of changing the underlying loader rules, we should set output.target to 'web' in the Rslib config.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Checklist