feat(web-platform): migrate environment.web output format and update tests for runtime wrapper plugin #1677
Conversation
🦋 Changeset detectedLatest commit: 5ba7a50 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 |
📝 WalkthroughWalkthroughMoves the runtime wrapper plugin to apply across all environments, updates web test to expect that wrapper, adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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! |
CodSpeed Performance ReportMerging #1677 will degrade performances by 9.7%Comparing Summary
Benchmarks breakdown
Footnotes
|
Web Explorer#5402 Bundle Size — 365.49KiB (~+0.01%).5ba7a50(current) vs d523f89 main#5401(baseline) Bundle metrics
|
| Current #5402 |
Baseline #5401 |
|
|---|---|---|
 Initial JS |
145.66KiB(~+0.01%) |
145.65KiB |
 Initial CSS |
32KiB |
32KiB |
 Cache Invalidation |
39.85% |
0% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
| Modules |
218(-0.46%) |
219 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
3.37% |
3.37% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #5402 |
Baseline #5401 |
|
|---|---|---|
| JS |
239.48KiB (~+0.01%) |
239.46KiB |
| Other |
94.02KiB |
94.02KiB |
| CSS |
32KiB |
32KiB |
Bundle analysis report Branch PupilTong:p/hw/enable-wrapper Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#5409 Bundle Size — 237.66KiB (0%).5ba7a50(current) vs d523f89 main#5408(baseline) Bundle metrics
|
| Current #5409 |
Baseline #5408 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
165 |
165 |
| Duplicate Modules |
67 |
67 |
| Duplicate Code |
46.76% |
46.76% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #5409 |
Baseline #5408 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.91KiB |
91.91KiB |
Bundle analysis report Branch PupilTong:p/hw/enable-wrapper Project dashboard
Generated by RelativeCI Documentation Report issue
PupilTong
force-pushed
the
p/hw/enable-wrapper
branch
5 times, most recently
from
9d1bd80 to
b9f6606
Compare
PupilTong
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts (1)
68-81: EnsureappTypereachesencodeOptionsused byhooks.encode.You assign
appTypeonencodeData, buthooks.encodereads it fromencodeOptions. To avoidundefinedin the final payload, also copy it ontoencodeOptionshere.Apply this diff inside the same callback:
Object.assign(encodeData, { styleInfo, manifest: { // `app-service.js` is the entry point of a template. '/app-service.js': content, }, customSections: encodeData.customSections, cardType: encodeData.sourceContent.dsl.substring(0, 5), - appType: encodeData.sourceContent.appType, + appType: encodeData.sourceContent.appType, pageConfig: { ...encodeData.compilerOptions, ...encodeData.sourceContent.config, }, }); + // keep parity with other top-level fields consumed in `hooks.encode` + // and prevent `appType` from being `undefined` there. + // (JSON.stringify drops `undefined`, which can silently hide the field.) + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (encodeOptions as any).appType = encodeData.sourceContent.appType;packages/rspeedy/plugin-react/src/splitChunks.ts (1)
80-95: Don’t override user-defined splitChunks.chunks.Respect existing user config (e.g., 'async') and only set the function when undefined.
- api.modifyRspackConfig((rspackConfig) => { - if (!rspackConfig.optimization) { - return rspackConfig - } - - if (!rspackConfig.optimization.splitChunks) { - return rspackConfig - } - - rspackConfig.optimization.splitChunks.chunks = function chunks(chunk) { - // TODO: support `splitChunks.chunks: 'async'` - // We don't want main thread to be split - return !chunk.name?.includes('__main-thread') - } - return rspackConfig - }) + api.modifyRspackConfig((rspackConfig) => { + if (!rspackConfig.optimization || !rspackConfig.optimization.splitChunks) { + return rspackConfig + } + if (rspackConfig.optimization.splitChunks === false) { + return rspackConfig + } + const sc = rspackConfig.optimization.splitChunks as Exclude<SplitChunks, false> + // Respect user-provided value, including literals like 'async' + if (typeof sc === 'object' && 'chunks' in sc && sc.chunks !== undefined) { + return rspackConfig + } + sc.chunks = function chunks(chunk) { + // TODO: support `splitChunks.chunks: 'async'` + // We don't want main thread to be split + return !chunk.name?.includes('__main-thread') + } + return rspackConfig + })
🧹 Nitpick comments (11)
packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts (2)
89-105: Avoid magic number for the schema version.Declare a single source of truth for the payload version to ease future bumps and cross-plugin reuse.
Apply this diff:
- version: 2, + version: WebEncodePlugin.VERSION,And add outside this range:
// near class definition export class WebEncodePlugin { static name = 'WebEncodePlugin'; static BEFORE_ENCODE_HOOK_STAGE = 100; static ENCODE_HOOK_STAGE = 100; static VERSION = 2; // payload schema version ... }
68-81: Optional: default/validateappType.If
sourceContent.appTypecan be absent, consider defaulting or validating to prevent downstream assumptions.Would you like me to add a lightweight type-guard here that logs in debug mode when
appTypeis missing?.changeset/plenty-actors-cheer.md (2)
8-8: Use a heading instead of emphasis for the breaking change note.Fixes markdownlint MD036 and makes the notice clearer in rendered views.
Apply this diff:
-**Breaking Change, you will need to upgrade your web platform version** +## Breaking changes + +You will need to upgrade your web platform version.
16-19: Tighten language and grammar.Minor copyedits for clarity.
Apply this diff:
-- use lynx.requireModuleAsync/lynx.requireModule to load Javascript Chunk -- be added a IIFE function wrapper for bts chunks -- mts chunk cannot be splitted -- some global variables, is exported by the IIFE wrapper +- Use lynx.requireModuleAsync/lynx.requireModule to load JavaScript chunks +- BTS chunks are wrapped in an IIFE +- MTS chunks cannot be split +- Some globals are exported by the IIFE wrapperpackages/web-platform/web-tests/tests/react.spec.ts (3)
1454-1455: Include a skip reason in the report.Add a reason to show up in Playwright output.
Apply this diff:
-test.skip(); // mts chunk split: not yet implemented +test.skip(true, 'mts chunk split: not yet implemented');
1464-1467: Add explicit reason for SSR-based skip.Apply this diff:
- test.skip( - isSSR, - ); + test.skip(isSSR, 'No need to test on SSR');
1476-1479: Add explicit reason for SSR-based skip.Apply this diff:
- test.skip( - isSSR, - ); + test.skip(isSSR, 'No need to test on SSR');packages/rspeedy/plugin-react/src/entry.ts (1)
203-225: Reduce per-compilation allocations forUNUSED_VARS.Hoist the set to a module-level const to avoid recreating it each compile.
Apply this diff:
- injectVars(vars) { - const UNUSED_VARS = new Set([ - 'Card', - 'Component', - 'ReactLynx', - 'Behavior', - ]) + injectVars(vars) { + // defined once at module scope return vars.map(name => { - if (UNUSED_VARS.has(name)) { + if (UNUSED_RUNTIME_VARS.has(name)) { return `__${name}` } return name }) },And add outside this range at top-level:
const UNUSED_RUNTIME_VARS = new Set(['Card', 'Component', 'ReactLynx', 'Behavior']);packages/rspeedy/core/test/plugins/chunkLoading.plugin.test.ts (1)
14-21: Also assert LynxCacheEventsPlugin presence.Since the plugin wires 'lynx:cache-events' unconditionally, add a check to guard regressions.
Example addition:
expect( config.plugins?.some(p => p && p.constructor.name === 'LynxCacheEventsPlugin'), ).toBeTruthy()packages/rspeedy/plugin-react/test/web.test.ts (1)
65-101: Rename outdated test title to reflect current behavior.Web now uses LynxTemplatePlugin too; update the name to avoid confusion.
- test('should have template plugin for lynx but not for web', async () => { + test('should have template plugin for lynx and web', async () => {Also consider renaming the earlier test title at Line 15 to “…not have encode plugin for web” since it asserts LynxEncodePlugin absence.
packages/rspeedy/plugin-react/test/config.test.ts (1)
1822-1828: Optionally validate chunks() semantics.Lightly sanity-check that the function returns false for “__main-thread” and true otherwise.
const chunks = (config!.optimization!.splitChunks as any).chunks expect(chunks({ name: 'foo__main-thread' })).toBe(false) expect(chunks({ name: 'foo' })).toBe(true)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.changeset/plenty-actors-cheer.md(1 hunks)packages/rspeedy/core/src/plugins/chunkLoading.plugin.ts(1 hunks)packages/rspeedy/core/test/plugins/chunkLoading.plugin.test.ts(2 hunks)packages/rspeedy/plugin-react/src/entry.ts(1 hunks)packages/rspeedy/plugin-react/src/splitChunks.ts(1 hunks)packages/rspeedy/plugin-react/test/config.test.ts(1 hunks)packages/rspeedy/plugin-react/test/web.test.ts(2 hunks)packages/web-platform/web-tests/tests/react.spec.ts(3 hunks)packages/web-platform/web-tests/tests/web-core.test.ts(3 hunks)packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
packages/web-platform/web-tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Place Playwright E2E tests in the web platform’s web-tests suite
Files:
packages/web-platform/web-tests/tests/react.spec.tspackages/web-platform/web-tests/tests/web-core.test.ts
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
For contributions, always generate a changeset and commit the resulting markdown file(s)
Files:
.changeset/plenty-actors-cheer.md
🧠 Learnings (6)
📚 Learning: 2025-08-27T12:42:01.095Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1616
File: packages/webpack/cache-events-webpack-plugin/test/cases/not-cache-events/lazy-bundle/index.js:3-3
Timestamp: 2025-08-27T12:42:01.095Z
Learning: In webpack, properties like __webpack_require__.lynx_ce are injected during compilation/build time when webpack processes modules and generates bundles, not at runtime when dynamic imports execute. Tests for such properties don't need to wait for dynamic imports to complete.
Applied to files:
packages/rspeedy/plugin-react/test/web.test.tspackages/rspeedy/core/test/plugins/chunkLoading.plugin.test.tspackages/rspeedy/plugin-react/src/entry.tspackages/web-platform/web-tests/tests/web-core.test.ts
📚 Learning: 2025-08-06T13:28:57.182Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1453
File: vitest.config.ts:49-61
Timestamp: 2025-08-06T13:28:57.182Z
Learning: In the lynx-family/lynx-stack repository, the file `packages/rspeedy/create-rspeedy/template-react-vitest-rltl-js/vitest.config.js` is a template file for scaffolding new Rspeedy projects, not a test configuration that should be included in the main vitest projects array.
Applied to files:
packages/rspeedy/plugin-react/test/web.test.tspackages/rspeedy/core/test/plugins/chunkLoading.plugin.test.ts
📚 Learning: 2025-08-21T08:46:54.494Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1370
File: packages/webpack/cache-events-webpack-plugin/src/LynxCacheEventsRuntimeModule.ts:23-27
Timestamp: 2025-08-21T08:46:54.494Z
Learning: In Lynx webpack runtime modules, the team prioritizes performance and simplicity over defensive runtime error handling. They prefer relying on compile-time type safety (TypeScript) rather than adding runtime checks like try-catch blocks or type validation, especially for performance-critical code like cache event setup/cleanup functions.
Applied to files:
packages/rspeedy/core/test/plugins/chunkLoading.plugin.test.tspackages/rspeedy/plugin-react/src/entry.tspackages/rspeedy/core/src/plugins/chunkLoading.plugin.ts
📚 Learning: 2025-08-27T08:10:09.932Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1612
File: packages/rspeedy/create-rspeedy/template-react-vitest-rltl-ts/src/tsconfig.json:3-13
Timestamp: 2025-08-27T08:10:09.932Z
Learning: In the lynx-family/lynx-stack repository, Rspeedy templates use `lynx-js/rspeedy/client` types via `rspeedy-env.d.ts` instead of `vite/client` types. Rspeedy provides its own client-side environment type definitions and doesn't require direct Vite type references.
Applied to files:
packages/rspeedy/core/test/plugins/chunkLoading.plugin.test.ts
📚 Learning: 2025-08-12T16:09:32.413Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1497
File: packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_full_static.js:9-10
Timestamp: 2025-08-12T16:09:32.413Z
Learning: In the Lynx stack, functions prefixed with `__` that are called in transformed code may be injected globally by the Lynx Engine at runtime rather than exported from the React runtime package. For example, `__CreateFrame` is injected globally by the Lynx Engine, not exported from lynx-js/react.
Applied to files:
packages/rspeedy/plugin-react/src/entry.ts
📚 Learning: 2025-09-09T12:38:10.439Z
Learnt from: CR
PR: lynx-family/lynx-stack#0
File: AGENTS.md:0-0
Timestamp: 2025-09-09T12:38:10.439Z
Learning: Applies to packages/web-platform/web-tests/** : Place Playwright E2E tests in the web platform’s web-tests suite
Applied to files:
packages/web-platform/web-tests/tests/web-core.test.ts
🧬 Code graph analysis (2)
packages/rspeedy/plugin-react/src/entry.ts (1)
packages/webpack/template-webpack-plugin/src/LynxEncodePlugin.ts (2)
name(289-294)name(296-316)
packages/rspeedy/core/src/plugins/chunkLoading.plugin.ts (2)
packages/webpack/chunk-loading-webpack-plugin/src/index.ts (1)
ChunkLoadingWebpackPlugin(58-87)packages/webpack/cache-events-webpack-plugin/src/LynxCacheEventsPlugin.ts (1)
LynxCacheEventsPlugin(58-96)
🪛 markdownlint-cli2 (0.17.2)
.changeset/plenty-actors-cheer.md
8-8: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: build / Build (Windows)
- GitHub Check: test-rust / Test (Ubuntu)
🔇 Additional comments (7)
packages/web-platform/web-tests/tests/web-core.test.ts (1)
107-110: Firefox-only skip with multi-thread guard — looks good.The conditional skip plus reason improves signal while keeping coverage elsewhere. Please ensure CI defines ENABLE_MULTI_THREAD consistently (e.g., "1"/"true").
Also applies to: 130-133, 161-164
packages/rspeedy/plugin-react/src/entry.ts (1)
221-224: Regex looks correct; verify hashed and non-hashed main-thread names.The negative lookahead excludes
main-thread.jsandmain-thread.[hash].js. Please confirm both dev (no hash) and prod (hashed) builds produce expected filenames and remain unwrapped.Do you want a small unit test asserting wrapper exclusion via emitted asset names?
packages/rspeedy/core/src/plugins/chunkLoading.plugin.ts (1)
14-26: Confirm web compatibility of chunkFormat('commonjs') + chunkLoading('lynx').This is now unconditional. Please re-verify that plain web builds (no Lynx env) still execute correctly (dynamic import, HMR, dev server) with this combo, or document the expectation. If needed, consider gating chunkFormat for non-Lynx targets or ensuring the runtime wrapper fully bridges this on web.
packages/rspeedy/core/test/plugins/chunkLoading.plugin.test.ts (2)
33-33: LGTM: assertion now focuses on chunkFormat for web.Matches the unconditional configuration in the plugin.
47-47: LGTM: multi-env web assertion updated to chunkFormat.Consistent with unified output settings.
packages/rspeedy/plugin-react/test/web.test.ts (1)
39-63: LGTM: runtime wrapper now expected on web.This aligns with applying the wrapper globally.
packages/rspeedy/plugin-react/test/config.test.ts (1)
1859-1860: LGTM: assert chunks as a function.Matches the new splitChunks behavior.
colinaaa
left a comment
colinaaa
left a comment
There was a problem hiding this comment.
I would prefer the following way (try our best to avoid breaking changes):
- In react-rsbuild-plugin, add RuntimeWrapperWebpackPlugin based on
output.chunkLoading:
if (output.chunkLoading === 'lynx') {
appendPlugin(RuntimeWrapperWebpackPlugin)
}- In Rspeedy, add ChunkLoadingWebpackPlugin in web environments.
In this way, the user could opt-in to use the new chunk loading by
- Upgrade Rspeedy and react-rsbuild-plugin
- Set
tools.rspack.output.chunkLoadingto'lynx'
And we avoid making BREAKING CHANGES to the existing projects.
We may change the default value later when we found this stable.
| const enableChunkSplitting = | ||
| rsbuildConfig.performance?.chunkSplit?.strategy !== 'all-in-one' | ||
|
|
||
| chain |
There was a problem hiding this comment.
This is also a BREAKING CHANGE. The new version of plugin would not work with old Rspeedy (with output.chunkLoading: 'import-scripts').
Either:
- Mark this as a minor change for the plugin.
- Decide whether to use
RuntimeWrapperWebpackPluginbased onoutput.chunkLoading
| }, | ||
| customSections: encodeData.customSections, | ||
| cardType: encodeData.sourceContent.dsl.substring(0, 5), | ||
| appType: encodeData.sourceContent.appType, |
There was a problem hiding this comment.
We need a changeset for this. Otherwise, the template plugin would not be published.
| .use(LynxCacheEventsPlugin) | ||
| .end() | ||
| .output | ||
| .chunkLoading('lynx') |
There was a problem hiding this comment.
I think it would be better to not change the value of chunkLoading in rspeedy/core. This could avoid making a breaking change.
.changeset/plenty-actors-cheer.md
Outdated
| "@lynx-js/rspeedy": minor | ||
| --- | ||
|
|
||
| feat(web-platform): use same chunk loading and chunk format with iOS/Android |
There was a problem hiding this comment.
We should do better on this changeset (CHANGELOG).
-
If we are making BREAKING CHANGE for two packages, try to split the changeset so that we could know what has changed for this package and how to migrate.
-
Make sure this is for end-user, not maintainer. I don't think most of the user would care about "chunk format" or "IIFE function wrapper".
|
|
||
| feat(web-platform): use same chunk loading and chunk format with iOS/Android | ||
|
|
||
| **Breaking Change, you will need to upgrade your web platform version** |
There was a problem hiding this comment.
This is not the only BREAKING CHANGE. The react-rsbuild-plugin would also require @lynx-js/rspeedy v0.12.0 (the new minor version).
PupilTong
force-pushed
the
p/hw/enable-wrapper
branch
from
b9f6606 to
1e4a8b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
.changeset/plenty-actors-cheer.md (2)
2-4: Split the changeset per package and call out cross-package compatibility explicitly.Two packages are bumped in one entry. To improve CHANGELOG clarity and migration, create two changeset files so each package has its own user-facing note. Also, explicitly state that the new react-rsbuild-plugin version requires the corresponding rspeedy minor (and add/verify a peerDependencies range).
Run this to locate and inspect dependent ranges and prepare separate changesets:
#!/bin/bash # Find all package.json that declare @lynx-js/rspeedy / react-rsbuild-plugin fd -a package.json | xargs -I{} jq -r '.name, .version, .dependencies["@lynx-js/rspeedy"], .peerDependencies["@lynx-js/rspeedy"], .dependencies["@lynx-js/react-rsbuild-plugin"], .peerDependencies["@lynx-js/react-rsbuild-plugin"] // empty' {} | paste - - - - - - # Grep for existing changesets that touch these packages (to avoid duplication) rg -nP '^\s*"(?:@lynx-js/(?:rspeedy|react-rsbuild-plugin))":\s*(?:patch|minor|major)\s*$' .changeset -C2
10-13: Add concrete migration steps and minimum versions.Call out the minimum Web Platform/runtime version and required package versions, and list actionable steps.
-Before this commit, the environment.web output has its own chunk format. - -After this commit, we migrate those into the format running on iOS/Android. +Previously, environment.web used a different chunk format. + +Now, Web uses the same runtime chunk loader/format as iOS/Android. + +### Migration +- Upgrade Web Platform/runtime to the required version (specify exact version). +- Upgrade `@lynx-js/rspeedy` to the new minor (specify). +- Upgrade `@lynx-js/react-rsbuild-plugin` to the new minor (specify) and ensure its peer dependency on rspeedy is satisfied. +- If you customized `output.chunkLoading` or `output.chunkFormat`, remove overrides or set: `chunkLoading: 'lynx'`, `chunkFormat: 'commonjs'`.packages/rspeedy/plugin-react/src/entry.ts (1)
203-225: Gate RuntimeWrapper for Web/Lynx and hoist UNUSED_VARSApplying the runtime wrapper unconditionally can affect non-Web/Lynx or custom/older targets; restrict it to Web or Lynx and avoid reallocating the unused-vars set on every call.
File: packages/rspeedy/plugin-react/src/entry.ts (around the RuntimeWrapper plugin block)
- chain - .plugin(PLUGIN_NAME_RUNTIME_WRAPPER) - .use(RuntimeWrapperWebpackPlugin, [{ + const shouldWrapRuntime = isLynx || isWeb + + chain + .when(shouldWrapRuntime, (c) => + c.plugin(PLUGIN_NAME_RUNTIME_WRAPPER) + .use(RuntimeWrapperWebpackPlugin, [{ injectVars(vars) { - const UNUSED_VARS = new Set([ - 'Card', - 'Component', - 'ReactLynx', - 'Behavior', - ]) + // defined above to avoid per-call allocation + // see: UNUSED_VARS declaration below return vars.map(name => { if (UNUSED_VARS.has(name)) { return `__${name}` } return name }) }, targetSdkVersion, - // Inject runtime wrapper for all `.js` but not `main-thread.js` and `main-thread.[hash].js`. - test: /^(?!.*main-thread(?:\.[A-Fa-f0-9]*)?\.js$).*\.js$/, + // Apply to all .js except paths ending with main-thread(.<hash>).js + test: /^(?!.*(?:^|\/)main-thread(?:\.[A-Fa-f0-9]+)?\.js$).*\.js$/, experimental_isLazyBundle, - }]) - .end() + }]) + .end() + )Hoist suggestion (place near other consts in this scope):
const UNUSED_VARS = new Set(['Card','Component','ReactLynx','Behavior'])
🧹 Nitpick comments (3)
.changeset/plenty-actors-cheer.md (3)
6-6: Use a user-facing title.Focus on impact rather than internals; e.g., “Web: dynamic imports now load with Lynx runtime (aligned with iOS/Android)”.
-feat(web-platform): use same chunk loading and chunk format with iOS/Android +Web: align dynamic import loading with iOS/Android (via Lynx runtime)
8-8: Fix Markdown style and lint error (MD036).Use a heading, not bold emphasis.
-**Breaking Change, you will need to upgrade your web platform version** +## Breaking changes +You need to upgrade your Web Platform version.
16-19: Tighten wording, fix grammar, and clarify terminology.Improve readability and define BTS/MTS terms.
-- use lynx.requireModuleAsync/lynx.requireModule to load Javascript Chunk -- be added a IIFE function wrapper for bts chunks -- mts chunk cannot be splitted -- some global variables, is exported by the IIFE wrapper +- Use `lynx.requireModuleAsync` / `lynx.requireModule` to load JavaScript chunks. +- Wrap Background Thread Script (BTS) chunks in an IIFE. +- Do not split Main Thread Script (MTS) chunks. +- Some globals are exported by the IIFE wrapper (list them here if applicable).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/plenty-actors-cheer.md(1 hunks)packages/rspeedy/core/src/plugins/chunkLoading.plugin.ts(1 hunks)packages/rspeedy/core/test/plugins/chunkLoading.plugin.test.ts(2 hunks)packages/rspeedy/plugin-react/src/entry.ts(1 hunks)packages/rspeedy/plugin-react/test/web.test.ts(2 hunks)packages/web-platform/web-tests/tests/react.spec.ts(3 hunks)packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/rspeedy/core/src/plugins/chunkLoading.plugin.ts
- packages/rspeedy/core/test/plugins/chunkLoading.plugin.test.ts
- packages/rspeedy/plugin-react/test/web.test.ts
- packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts
- packages/web-platform/web-tests/tests/react.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
For contributions, always generate a changeset and commit the resulting markdown file(s)
Files:
.changeset/plenty-actors-cheer.md
🧠 Learnings (4)
📚 Learning: 2025-08-12T16:09:32.413Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1497
File: packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_full_static.js:9-10
Timestamp: 2025-08-12T16:09:32.413Z
Learning: In the Lynx stack, functions prefixed with `__` that are called in transformed code may be injected globally by the Lynx Engine at runtime rather than exported from the React runtime package. For example, `__CreateFrame` is injected globally by the Lynx Engine, not exported from lynx-js/react.
Applied to files:
packages/rspeedy/plugin-react/src/entry.ts
📚 Learning: 2025-08-27T12:42:01.095Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1616
File: packages/webpack/cache-events-webpack-plugin/test/cases/not-cache-events/lazy-bundle/index.js:3-3
Timestamp: 2025-08-27T12:42:01.095Z
Learning: In webpack, properties like __webpack_require__.lynx_ce are injected during compilation/build time when webpack processes modules and generates bundles, not at runtime when dynamic imports execute. Tests for such properties don't need to wait for dynamic imports to complete.
Applied to files:
packages/rspeedy/plugin-react/src/entry.ts
📚 Learning: 2025-08-21T08:46:54.494Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1370
File: packages/webpack/cache-events-webpack-plugin/src/LynxCacheEventsRuntimeModule.ts:23-27
Timestamp: 2025-08-21T08:46:54.494Z
Learning: In Lynx webpack runtime modules, the team prioritizes performance and simplicity over defensive runtime error handling. They prefer relying on compile-time type safety (TypeScript) rather than adding runtime checks like try-catch blocks or type validation, especially for performance-critical code like cache event setup/cleanup functions.
Applied to files:
packages/rspeedy/plugin-react/src/entry.ts
📚 Learning: 2025-07-22T09:23:07.797Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:23:07.797Z
Learning: In the lynx-family/lynx-stack repository, changesets are only required for meaningful changes to end-users such as bugfixes and features. Internal/development changes like chores, refactoring, or removing debug info do not need changeset entries.
Applied to files:
.changeset/plenty-actors-cheer.md
🧬 Code graph analysis (1)
packages/rspeedy/plugin-react/src/entry.ts (1)
packages/webpack/template-webpack-plugin/src/LynxEncodePlugin.ts (2)
name(289-294)name(296-316)
🪛 markdownlint-cli2 (0.17.2)
.changeset/plenty-actors-cheer.md
8-8: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.changeset/plenty-actors-cheer.md (1)
1-4: Split changeset and tailor migration per package.Prior feedback asked to split when multiple packages have breaking surface and to write end‑user guidance, not maintainer jargon. Also call out the react‑rsbuild‑plugin’s minimum rspeedy version explicitly.
Proposed actions:
- Create two changeset files (one per package) with package‑specific “Who is affected” and “How to migrate”.
- Add “peer/minimum version” note: react‑rsbuild‑plugin requires @lynx-js/rspeedy ≥ 0.12.0 (or the correct version if different).
🧹 Nitpick comments (5)
.changeset/plenty-actors-cheer.md (2)
8-8: Use a proper BREAKING CHANGES heading (and fix MD036).Apply:
-**Breaking Change, you will need to upgrade your web platform version** +### BREAKING CHANGES + +You must upgrade your web platform version.
16-19: Clarify bullets, fix grammar, and write for end‑users.Apply:
-- use lynx.requireModuleAsync/lynx.requireModule to load Javascript Chunk -- be added a IIFE function wrapper for bts chunks -- mts chunk cannot be splitted -- some global variables, is exported by the IIFE wrapper +- Chunks load via `lynx.requireModuleAsync` / `lynx.requireModule`. +- BTS chunks are wrapped in an IIFE (self‑invoking) wrapper. +- MTS chunks are no longer split. +- The wrapper exports selected globals (documented below).Also add an “Upgrade and migration” section:
+### Migration + +- Default web output now uses `chunkLoading: 'lynx'` and `chunkFormat: 'commonjs'`, aligned with iOS/Android. +- If your web build relied on native `import` chunk loading, override in rspeedy config: + + ```js + // rspeedy.config.ts + export default { + environments: { web: { tools: { rspack: { output: { chunkLoading: 'import' } } } } } + } + ``` + +- Validate that any code assuming ESM wrapper semantics still works with the IIFE + CommonJS runtime.packages/rspeedy/core/src/plugins/chunkLoading.plugin.ts (1)
14-26: Optional: leave a brief rationale comment and point to override.api.modifyBundlerChain((chain) => { - // dprint-ignore + // Unify all environments with Lynx chunk loading/CommonJS; users can override via tools.rspack.output. + // dprint-ignore chainpackages/rspeedy/core/test/plugins/chunkLoading.plugin.test.ts (2)
33-35: Assert chunkLoading on Web too (locks the contract).- expect(config.output?.chunkFormat).toBe('commonjs') + expect(config.output?.chunkLoading).toBe('lynx') + expect(config.output?.chunkFormat).toBe('commonjs')
47-52: Also assert webConfig chunkLoading is 'lynx'.- expect(webConfig?.output?.chunkFormat).toBe('commonjs') + expect(webConfig?.output?.chunkLoading).toBe('lynx') + expect(webConfig?.output?.chunkFormat).toBe('commonjs')
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/plenty-actors-cheer.md(1 hunks)packages/rspeedy/core/src/plugins/chunkLoading.plugin.ts(1 hunks)packages/rspeedy/core/test/plugins/chunkLoading.plugin.test.ts(2 hunks)packages/rspeedy/plugin-react/src/entry.ts(1 hunks)packages/rspeedy/plugin-react/test/web.test.ts(2 hunks)packages/web-platform/web-tests/tests/react.spec.ts(3 hunks)packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/web-platform/web-tests/tests/react.spec.ts
- packages/rspeedy/plugin-react/test/web.test.ts
- packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts
- packages/rspeedy/plugin-react/src/entry.ts
🧰 Additional context used
📓 Path-based instructions (1)
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
For contributions, always generate a changeset and commit the resulting markdown file(s)
Files:
.changeset/plenty-actors-cheer.md
🧠 Learnings (4)
📚 Learning: 2025-08-27T12:42:01.095Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1616
File: packages/webpack/cache-events-webpack-plugin/test/cases/not-cache-events/lazy-bundle/index.js:3-3
Timestamp: 2025-08-27T12:42:01.095Z
Learning: In webpack, properties like __webpack_require__.lynx_ce are injected during compilation/build time when webpack processes modules and generates bundles, not at runtime when dynamic imports execute. Tests for such properties don't need to wait for dynamic imports to complete.
Applied to files:
packages/rspeedy/core/test/plugins/chunkLoading.plugin.test.ts
📚 Learning: 2025-08-21T08:46:54.494Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1370
File: packages/webpack/cache-events-webpack-plugin/src/LynxCacheEventsRuntimeModule.ts:23-27
Timestamp: 2025-08-21T08:46:54.494Z
Learning: In Lynx webpack runtime modules, the team prioritizes performance and simplicity over defensive runtime error handling. They prefer relying on compile-time type safety (TypeScript) rather than adding runtime checks like try-catch blocks or type validation, especially for performance-critical code like cache event setup/cleanup functions.
Applied to files:
packages/rspeedy/core/test/plugins/chunkLoading.plugin.test.tspackages/rspeedy/core/src/plugins/chunkLoading.plugin.ts
📚 Learning: 2025-08-27T08:10:09.932Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1612
File: packages/rspeedy/create-rspeedy/template-react-vitest-rltl-ts/src/tsconfig.json:3-13
Timestamp: 2025-08-27T08:10:09.932Z
Learning: In the lynx-family/lynx-stack repository, Rspeedy templates use `lynx-js/rspeedy/client` types via `rspeedy-env.d.ts` instead of `vite/client` types. Rspeedy provides its own client-side environment type definitions and doesn't require direct Vite type references.
Applied to files:
packages/rspeedy/core/test/plugins/chunkLoading.plugin.test.ts
📚 Learning: 2025-08-06T13:28:57.182Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1453
File: vitest.config.ts:49-61
Timestamp: 2025-08-06T13:28:57.182Z
Learning: In the lynx-family/lynx-stack repository, the file `packages/rspeedy/create-rspeedy/template-react-vitest-rltl-js/vitest.config.js` is a template file for scaffolding new Rspeedy projects, not a test configuration that should be included in the main vitest projects array.
Applied to files:
packages/rspeedy/core/test/plugins/chunkLoading.plugin.test.ts
🧬 Code graph analysis (1)
packages/rspeedy/core/src/plugins/chunkLoading.plugin.ts (2)
packages/webpack/chunk-loading-webpack-plugin/src/index.ts (1)
ChunkLoadingWebpackPlugin(58-87)packages/webpack/cache-events-webpack-plugin/src/LynxCacheEventsPlugin.ts (1)
LynxCacheEventsPlugin(58-96)
🪛 markdownlint-cli2 (0.17.2)
.changeset/plenty-actors-cheer.md
8-8: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (2)
.changeset/plenty-actors-cheer.md (1)
2-4: SemVer mismatch — declare Breaking but versions set to minor.changeset/plenty-actors-cheer.md (lines 2–4) declares "Breaking Change, you will need to upgrade your web platform version" while both packages are set to minor. Either bump to major or remove the breaking language.
File: .changeset/plenty-actors-cheer.md (lines 2–4)
Option A (major bump):
--- -"@lynx-js/react-rsbuild-plugin": minor -"@lynx-js/rspeedy": minor +"@lynx-js/react-rsbuild-plugin": major +"@lynx-js/rspeedy": major ---Option B (no major): keep minor but replace “Breaking Change” with “Migration” and document opt‑out/override and the repo's release policy.
⛔ Skipped due to learnings
Learnt from: upupming PR: lynx-family/lynx-stack#1370 File: .changeset/brave-melons-add.md:1-7 Timestamp: 2025-08-14T12:54:51.143Z Learning: In the lynx-family/lynx-stack repository, packages use 0.x.x versioning where minor version bumps indicate breaking changes (not major bumps), following pre-1.0 semantic versioning conventions.Learnt from: colinaaa PR: lynx-family/lynx-stack#1612 File: packages/rspeedy/create-rspeedy/template-react-vitest-rltl-ts/src/tsconfig.json:3-13 Timestamp: 2025-08-27T08:10:09.932Z Learning: In the lynx-family/lynx-stack repository, Rspeedy templates use `lynx-js/rspeedy/client` types via `rspeedy-env.d.ts` instead of `vite/client` types. Rspeedy provides its own client-side environment type definitions and doesn't require direct Vite type references.Learnt from: colinaaa PR: lynx-family/lynx-stack#1558 File: .changeset/solid-squids-fall.md:2-2 Timestamp: 2025-08-19T11:25:36.127Z Learning: In the lynx-family/lynx-stack repository, changesets should use the exact package name from package.json#name, not generic or unscoped names. Each package has its own specific scoped name (e.g., "lynx-js/react-transform" for packages/react/transform).Learnt from: colinaaa PR: lynx-family/lynx-stack#1566 File: package.json:53-53 Timestamp: 2025-08-20T04:56:36.011Z Learning: In lynx-stack, Node.js v24 is the preferred/default version for development (established in PR #1557), but Node.js v22 compatibility is maintained specifically for external CI systems like rspack-ecosystem-ci. The engines.node specification uses "^22 || ^24" to support both versions while keeping v24 as the primary target.Learnt from: colinaaa PR: lynx-family/lynx-stack#1523 File: vitest.config.ts:52-72 Timestamp: 2025-08-13T11:36:12.075Z Learning: The lynx-stack project requires Node.js >=22 as specified in package.json engines, so Node.js compatibility fallbacks for features introduced before v22 are unnecessary.packages/rspeedy/core/src/plugins/chunkLoading.plugin.ts (1)
14-26: LGTM: unified chunk loader and format, plus required plugins wired.
PupilTong
force-pushed
the
p/hw/enable-wrapper
branch
from
1e4a8b1 to
80dcd65
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rspeedy/plugin-react/src/entry.ts (1)
316-319: Bug: self-append duplicatesimportsexponentially.
imports.push(...imports)duplicates the array each time an array entry is encountered; likely intended to spread the currentitem.- if (Array.isArray(item)) { - imports.push(...imports) - continue - } + if (Array.isArray(item)) { + imports.push(...item) + continue + }
♻️ Duplicate comments (2)
.changeset/plenty-actors-cheer.md (2)
2-4: Split changes by package and call out peer-dep constraints explicitly.Two packages are bumped together with shared BREAKING changes. Please create separate changesets (one per package) so migration notes are package-scoped, and declare the peer dependency requirement (e.g., react-rsbuild-plugin requires the new @lynx-js/rspeedy minor). This mirrors prior feedback.
If helpful, I can draft the additional changeset file(s) with accurate package names/versions once you confirm the intended peer ranges.
6-19: Rewrite for end-users, fix grammar, and address MD036.Current text mixes internal terms (IIFE, chunk format) and has grammar issues; markdownlint flags “Emphasis used instead of a heading” on Line 8. Propose end-user–focused headings, clear migration steps, and grammar fixes.
feat(web-platform): use same chunk loading and chunk format with iOS/Android - -**Breaking Change, you will need to upgrade your web platform version** - -Before this commit, the environment.web output has its own chunk format. - -After this commit, we migrate those into the format running on iOS/Android. - -The new format has such special features: - -- use lynx.requireModuleAsync/lynx.requireModule to load Javascript Chunk -- be added a IIFE function wrapper for bts chunks -- mts chunk cannot be splitted -- some global variables, is exported by the IIFE wrapper + +### Breaking changes +The Web platform now uses Lynx chunk loading with CommonJS chunks, consistent with iOS/Android. You must upgrade your Web platform/runtime to the version compatible with this release. + +### What changed +- JavaScript chunks are loaded via `lynx.requireModuleAsync` / `lynx.requireModule`. +- BTS chunks are wrapped in an IIFE function wrapper. +- MTS chunks cannot be split. +- Some globals are exported by the IIFE wrapper. + +### Who is affected +- Projects targeting the Web platform using the previous `import-scripts` loader/format. +- Projects using `@lynx-js/react-rsbuild-plugin` with older `@lynx-js/rspeedy`. + +### Migration +1) Bump to this release of `@lynx-js/rspeedy` and `@lynx-js/react-rsbuild-plugin`. +2) Remove any overrides that force `tools.rspack.output.chunkLoading: 'import'`. +3) If you must temporarily stay on `import`, explicitly set `tools.rspack.output.chunkLoading: 'import'` and plan to migrate; note that some features (lazy bundle/runtime wrapper) may not apply. + +### Compatibility +- `@lynx-js/react-rsbuild-plugin` requires the corresponding minor of `@lynx-js/rspeedy` released with this change.
🧹 Nitpick comments (4)
packages/rspeedy/core/test/plugins/chunkLoading.plugin.test.ts (2)
33-35: Also assertchunkLoadingislynxfor web.Given the unified defaults, this test should verify
output.chunkLoadingas well to catch regressions.- expect(config.output?.chunkFormat).toBe('commonjs') + expect(config.output?.chunkFormat).toBe('commonjs') + expect(config.output?.chunkLoading).toBe('lynx')
47-52: AddchunkLoadingexpectation for the web config in multi-env.Ensures both envs are aligned on the new default.
- expect(webConfig?.output?.chunkFormat).toBe('commonjs') + expect(webConfig?.output?.chunkFormat).toBe('commonjs') + expect(webConfig?.output?.chunkLoading).toBe('lynx')packages/rspeedy/plugin-react/src/entry.ts (2)
203-225: Runtime wrapper now global — verify behavior when users overridechunkLoading.Unconditionally applying
RuntimeWrapperWebpackPluginmay break configs that settools.rspack.output.chunkLoading: 'import'(your tests allow this override). Either gate the wrapper onenvironment.config.output?.chunkLoading === 'lynx'or document the new requirement explicitly in the changeset and add tests for the override path.Option A (gate by chunkLoading):
- chain - .plugin(PLUGIN_NAME_RUNTIME_WRAPPER) + chain.when(environment.config.output?.chunkLoading === 'lynx', c => + c.plugin(PLUGIN_NAME_RUNTIME_WRAPPER) .use(RuntimeWrapperWebpackPlugin, [{ injectVars(vars) { - const UNUSED_VARS = new Set([ - 'Card', - 'Component', - 'ReactLynx', - 'Behavior', - ]) + const UNUSED_VARS = RUNTIME_WRAPPER_UNUSED_VARS return vars.map(name => { if (UNUSED_VARS.has(name)) { return `__${name}` } return name }) }, targetSdkVersion, - // Inject runtime wrapper for all `.js` but not `main-thread.js` and `main-thread.[hash].js`. - test: /^(?!.*main-thread(?:\.[A-Fa-f0-9]*)?\.js$).*\.js$/, + // Inject wrapper for all `.js` except `main-thread(.hash).js`. + test: /^(?!.*main-thread(?:\.[^/]+)?\.js$).*\.js$/, experimental_isLazyBundle, - }]) - .end() + }]).end() + )And hoist the constant once (outside this block):
const RUNTIME_WRAPPER_UNUSED_VARS = new Set(['Card', 'Component', 'ReactLynx', 'Behavior'])
251-255: Small wording nit in warning.Trailing period inside the backticked identifier reads oddly.
- logger.warn( - '`extractStr` is changed to `false` because it is only supported in `all-in-one` chunkSplit strategy, please set `performance.chunkSplit.strategy` to `all-in-one` to use `extractStr.`', - ) + logger.warn('`extractStr` is set to `false` because it is only supported when `performance.chunkSplit.strategy` is `all-in-one`. To use `extractStr`, set `performance.chunkSplit.strategy` to `all-in-one`.')
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/plenty-actors-cheer.md(1 hunks)packages/rspeedy/core/src/plugins/chunkLoading.plugin.ts(1 hunks)packages/rspeedy/core/test/plugins/chunkLoading.plugin.test.ts(2 hunks)packages/rspeedy/plugin-react/src/entry.ts(1 hunks)packages/rspeedy/plugin-react/test/web.test.ts(2 hunks)packages/web-platform/web-tests/tests/react.spec.ts(3 hunks)packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/rspeedy/plugin-react/test/web.test.ts
- packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts
- packages/web-platform/web-tests/tests/react.spec.ts
- packages/rspeedy/core/src/plugins/chunkLoading.plugin.ts
🧰 Additional context used
📓 Path-based instructions (1)
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
For contributions, always generate a changeset and commit the resulting markdown file(s)
Files:
.changeset/plenty-actors-cheer.md
🧠 Learnings (6)
📚 Learning: 2025-08-27T12:42:01.095Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1616
File: packages/webpack/cache-events-webpack-plugin/test/cases/not-cache-events/lazy-bundle/index.js:3-3
Timestamp: 2025-08-27T12:42:01.095Z
Learning: In webpack, properties like __webpack_require__.lynx_ce are injected during compilation/build time when webpack processes modules and generates bundles, not at runtime when dynamic imports execute. Tests for such properties don't need to wait for dynamic imports to complete.
Applied to files:
packages/rspeedy/core/test/plugins/chunkLoading.plugin.test.tspackages/rspeedy/plugin-react/src/entry.ts
📚 Learning: 2025-08-21T08:46:54.494Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1370
File: packages/webpack/cache-events-webpack-plugin/src/LynxCacheEventsRuntimeModule.ts:23-27
Timestamp: 2025-08-21T08:46:54.494Z
Learning: In Lynx webpack runtime modules, the team prioritizes performance and simplicity over defensive runtime error handling. They prefer relying on compile-time type safety (TypeScript) rather than adding runtime checks like try-catch blocks or type validation, especially for performance-critical code like cache event setup/cleanup functions.
Applied to files:
packages/rspeedy/core/test/plugins/chunkLoading.plugin.test.tspackages/rspeedy/plugin-react/src/entry.ts
📚 Learning: 2025-08-27T08:10:09.932Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1612
File: packages/rspeedy/create-rspeedy/template-react-vitest-rltl-ts/src/tsconfig.json:3-13
Timestamp: 2025-08-27T08:10:09.932Z
Learning: In the lynx-family/lynx-stack repository, Rspeedy templates use `lynx-js/rspeedy/client` types via `rspeedy-env.d.ts` instead of `vite/client` types. Rspeedy provides its own client-side environment type definitions and doesn't require direct Vite type references.
Applied to files:
packages/rspeedy/core/test/plugins/chunkLoading.plugin.test.ts
📚 Learning: 2025-08-12T16:09:32.413Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1497
File: packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_full_static.js:9-10
Timestamp: 2025-08-12T16:09:32.413Z
Learning: In the Lynx stack, functions prefixed with `__` that are called in transformed code may be injected globally by the Lynx Engine at runtime rather than exported from the React runtime package. For example, `__CreateFrame` is injected globally by the Lynx Engine, not exported from lynx-js/react.
Applied to files:
packages/rspeedy/plugin-react/src/entry.ts
📚 Learning: 2025-09-12T09:43:04.810Z
Learnt from: gaoachao
PR: lynx-family/lynx-stack#1736
File: .changeset/spotty-experts-smoke.md:1-3
Timestamp: 2025-09-12T09:43:04.810Z
Learning: In the lynx-family/lynx-stack repository, empty changeset files (containing only `---\n\n---`) are used for internal changes that modify src/** files but don't require meaningful release notes, such as private package changes or testing-only modifications. This satisfies CI requirements without generating user-facing release notes.
Applied to files:
.changeset/plenty-actors-cheer.md
📚 Learning: 2025-09-12T09:43:04.810Z
Learnt from: gaoachao
PR: lynx-family/lynx-stack#1736
File: .changeset/spotty-experts-smoke.md:1-3
Timestamp: 2025-09-12T09:43:04.810Z
Learning: In the lynx-family/lynx-stack repository, private packages (marked with "private": true in package.json) like lynx-js/react-transform don't require meaningful changeset entries even when their public APIs change, since they are not published externally and only affect internal development.
Applied to files:
.changeset/plenty-actors-cheer.md
🧬 Code graph analysis (1)
packages/rspeedy/plugin-react/src/entry.ts (1)
packages/webpack/template-webpack-plugin/src/LynxEncodePlugin.ts (2)
name(289-294)name(296-316)
🪛 markdownlint-cli2 (0.17.2)
.changeset/plenty-actors-cheer.md
8-8: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: build / Build (Windows)
- GitHub Check: test-rust / Test (Ubuntu)
PupilTong
force-pushed
the
p/hw/enable-wrapper
branch
from
80dcd65 to
30a9519
Compare
PupilTong
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.changeset/plenty-actors-cheer.md (2)
2-3: Changeset severity and content mismatch; make it end‑user focused, list all affected packages, and specify migration.The body calls out a Breaking Change but the release type is “minor”, and only one package is listed. Also, the text targets maintainers (chunk format/IIFE) rather than users and triggers MD036 (bold used as heading). Align severity, include all impacted packages (e.g., template plugin), and add a clear migration note including the required @lynx-js/rspeedy version.
Apply this rewrite (adjust versions/package names as appropriate):
--- -"@lynx-js/react-rsbuild-plugin": minor +"@lynx-js/react-rsbuild-plugin": major +"@lynx-js/template-webpack-plugin": minor --- -feat(web-platform): migrate environment.web output format and update tests for runtime wrapper plugin - -**Breaking Change, you will need to upgrade your web platform version** - -Before this commit, the environment.web output has its own chunk format. - -After this commit, we migrate those into the format running on iOS/Android. - -The new format has such special features: - -- some global variables, is exported by the IIFE wrapper +feat(web-platform): align environment.web output with iOS/Android + +### Breaking change +Web output format now matches iOS/Android. This requires upgrading your web platform and toolchain. + +### What changed (user-facing) +- Unified chunk loading/format across platforms for consistent runtime behavior. +- Certain symbols are now exposed via a small runtime wrapper to match mobile. + +### Migration +1) Upgrade @lynx-js/rspeedy to >= 0.12.0. +2) Upgrade your web platform to the version that supports the unified format. +3) No application code changes are expected for most projects. + +### Notes for maintainers +- @lynx-js/react-rsbuild-plugin now applies the runtime wrapper for web; older Rspeedy configurations may not be compatible. +- @lynx-js/template-webpack-plugin encodings bumped (web encode version v2).Option B (to keep a minor bump): gate RuntimeWrapper usage by output configuration to avoid breaking older setups. See my comment in packages/rspeedy/plugin-react/src/entry.ts for a concrete gating diff.
Also applies to: 5-16
2-3: Add peerDependency on @lynx-js/rspeedy >= 0.12.0 to react-rsbuild-plugin; template-webpack-plugin changeset present.
- packages/rspeedy/plugin-react/package.json (react-rsbuild-plugin): add "peerDependencies": { "@lynx-js/rspeedy": ">=0.12.0" } — currently missing.
- .changeset/loud-ways-refuse.md and .changeset/clever-maps-wait.md reference @lynx-js/template-webpack-plugin — no additional changeset needed for the template plugin.
🧹 Nitpick comments (2)
.changeset/plenty-actors-cheer.md (1)
7-7: Fix markdown style (MD036).Use a heading instead of bold for the “Breaking change” section.
-**Breaking Change, you will need to upgrade your web platform version** +### Breaking change +You will need to upgrade your web platform version.packages/rspeedy/plugin-react/src/entry.ts (1)
203-215: Hoist UNUSED_VARS to module scope.Tiny nit: avoid re-allocating the Set on each call; declare it once.
Outside this hunk, add at top-level (near other constants):
const UNUSED_VARS = new Set(['Card', 'Component', 'ReactLynx', 'Behavior'])Then inside injectVars just reference UNUSED_VARS.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/plenty-actors-cheer.md(1 hunks)packages/rspeedy/plugin-react/src/entry.ts(1 hunks)packages/rspeedy/plugin-react/test/web.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rspeedy/plugin-react/test/web.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
For contributions, always generate a changeset and commit the resulting markdown file(s)
Files:
.changeset/plenty-actors-cheer.md
🧠 Learnings (5)
📓 Common learnings
Learnt from: upupming
PR: lynx-family/lynx-stack#1616
File: packages/webpack/cache-events-webpack-plugin/test/cases/not-cache-events/lazy-bundle/index.js:3-3
Timestamp: 2025-08-27T12:42:01.095Z
Learning: In webpack, properties like __webpack_require__.lynx_ce are injected during compilation/build time when webpack processes modules and generates bundles, not at runtime when dynamic imports execute. Tests for such properties don't need to wait for dynamic imports to complete.
📚 Learning: 2025-09-18T04:43:54.407Z
Learnt from: gaoachao
PR: lynx-family/lynx-stack#1771
File: packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_component_with_static_sibling.js:2-2
Timestamp: 2025-09-18T04:43:54.407Z
Learning: In the lynx-family/lynx-stack repository, the `add_pure_comment` function in packages/react/transform/src/swc_plugin_compat/mod.rs (around lines 478-482) is specifically for `wrapWithLynxComponent` calls, not `createSnapshot` calls. The PURE comment injection for `createSnapshot` is handled separately in swc_plugin_snapshot/mod.rs.
Applied to files:
packages/rspeedy/plugin-react/src/entry.ts
📚 Learning: 2025-08-27T12:42:01.095Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1616
File: packages/webpack/cache-events-webpack-plugin/test/cases/not-cache-events/lazy-bundle/index.js:3-3
Timestamp: 2025-08-27T12:42:01.095Z
Learning: In webpack, properties like __webpack_require__.lynx_ce are injected during compilation/build time when webpack processes modules and generates bundles, not at runtime when dynamic imports execute. Tests for such properties don't need to wait for dynamic imports to complete.
Applied to files:
packages/rspeedy/plugin-react/src/entry.ts
📚 Learning: 2025-08-21T08:46:54.494Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1370
File: packages/webpack/cache-events-webpack-plugin/src/LynxCacheEventsRuntimeModule.ts:23-27
Timestamp: 2025-08-21T08:46:54.494Z
Learning: In Lynx webpack runtime modules, the team prioritizes performance and simplicity over defensive runtime error handling. They prefer relying on compile-time type safety (TypeScript) rather than adding runtime checks like try-catch blocks or type validation, especially for performance-critical code like cache event setup/cleanup functions.
Applied to files:
packages/rspeedy/plugin-react/src/entry.ts
📚 Learning: 2025-09-12T09:43:04.847Z
Learnt from: gaoachao
PR: lynx-family/lynx-stack#1736
File: .changeset/spotty-experts-smoke.md:1-3
Timestamp: 2025-09-12T09:43:04.847Z
Learning: In the lynx-family/lynx-stack repository, private packages (marked with "private": true in package.json) like lynx-js/react-transform don't require meaningful changeset entries even when their public APIs change, since they are not published externally and only affect internal development.
Applied to files:
.changeset/plenty-actors-cheer.md
🧬 Code graph analysis (1)
packages/rspeedy/plugin-react/src/entry.ts (1)
packages/webpack/template-webpack-plugin/src/LynxEncodePlugin.ts (2)
name(289-294)name(296-316)
🪛 markdownlint-cli2 (0.17.2)
.changeset/plenty-actors-cheer.md
7-7: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
| chain | ||
| .plugin(PLUGIN_NAME_RUNTIME_WRAPPER) | ||
| .use(RuntimeWrapperWebpackPlugin, [{ | ||
| injectVars(vars) { | ||
| const UNUSED_VARS = new Set([ | ||
| 'Card', | ||
| 'Component', | ||
| 'ReactLynx', | ||
| 'Behavior', | ||
| ]) | ||
| return vars.map(name => { | ||
| if (UNUSED_VARS.has(name)) { | ||
| return `__${name}` | ||
| } | ||
| return name | ||
| }) | ||
| }, | ||
| targetSdkVersion, | ||
| // Inject runtime wrapper for all `.js` but not `main-thread.js` and `main-thread.[hash].js`. | ||
| test: /^(?!.*main-thread(?:\.[A-Fa-f0-9]*)?\.js$).*\.js$/, | ||
| experimental_isLazyBundle, | ||
| }]) | ||
| .end() |
There was a problem hiding this comment.
Gate RuntimeWrapper to avoid unintended breakage or bump plugin as major.
Applying RuntimeWrapper globally can break non-target environments and older Rspeedy configs. Either ship a major release (preferred if this is intentional), or gate the plugin to known targets (web/lynx) or by output.chunkLoading to preserve compatibility.
Example gating by environment (minimal and explicit):
- chain
- .plugin(PLUGIN_NAME_RUNTIME_WRAPPER)
- .use(RuntimeWrapperWebpackPlugin, [{
+ if (isWeb || isLynx) {
+ chain
+ .plugin(PLUGIN_NAME_RUNTIME_WRAPPER)
+ .use(RuntimeWrapperWebpackPlugin, [{
injectVars(vars) {
const UNUSED_VARS = new Set([
'Card',
'Component',
'ReactLynx',
'Behavior',
])
return vars.map(name => {
if (UNUSED_VARS.has(name)) {
return `__${name}`
}
return name
})
},
targetSdkVersion,
// Inject runtime wrapper for all `.js` but not `main-thread.js` and `main-thread.[hash].js`.
test: /^(?!.*main-thread(?:\.[A-Fa-f0-9]*)?\.js$).*\.js$/,
experimental_isLazyBundle,
- }])
- .end()
+ }])
+ .end()
+ }Alternatively, gate by output mode to maintain backward compatibility with older builds:
const chunkLoading = environment.config?.output?.chunkLoading
const enableWrapper = (isWeb || isLynx) && chunkLoading !== 'import-scripts'
if (enableWrapper) { /* install plugin as above */ }🤖 Prompt for AI Agents
In packages/rspeedy/plugin-react/src/entry.ts around lines 199 to 221, the
RuntimeWrapperWebpackPlugin is applied unconditionally which can break
non-target environments or older configs; either publish this change as a major
bump or gate the plugin installation: detect target environment (e.g., isWeb or
isLynx) and check output.chunkLoading (reject 'import-scripts') before calling
.plugin(...). Update the code to compute enableWrapper = (isWeb || isLynx) &&
chunkLoading !== 'import-scripts' (or equivalent environment checks) and only
call .plugin(...).use(...) when enableWrapper is true, ensuring backward
compatibility.
🧩 Analysis chain
Confirm plugin ordering relative to encoding/template plugins.
Wrapper should run before encoding/template transforms. The current order likely works; please confirm no regressions in web/lynx by checking emitted assets and wrapper presence before encoding.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for runtime-wrapper plugin registration order in entry.ts:"
rg -n --color=never "plugin\\(PLUGIN_NAME_RUNTIME_WRAPPER\\)|PLUGIN_NAME_TEMPLATE|PLUGIN_NAME_WEB|LynxEncodePlugin" packages/rspeedy/plugin-react/src/entry.ts -n -C2
echo
echo "List of environments referenced (to double-check wrapper gating scope):"
rg -n --color=never "environment\\.name\\s*===\\s*'[^']+'" packages -U | sed -E "s/.*'([^']+)'.*/\\1/" | sort -uLength of output: 1379
Move runtime-wrapper to run before template/encoding plugins.
packages/rspeedy/plugin-react/src/entry.ts — template registered at line 161, runtime-wrapper at line 199, LynxEncodePlugin at line 233, WebEncodePlugin at line 240. Move the RuntimeWrapperWebpackPlugin registration before the PLUGIN_NAME_TEMPLATE/encode plugin registrations (or enforce explicit .before ordering) so the wrapper executes prior to encoding/template transforms.
🤖 Prompt for AI Agents
In packages/rspeedy/plugin-react/src/entry.ts around lines 199 to 221, the
RuntimeWrapperWebpackPlugin is registered after the template/encoding plugins
(which are registered at ~161, 233, 240) so the runtime wrapper runs too late;
move the RuntimeWrapperWebpackPlugin registration to appear before the
PLUGIN_NAME_TEMPLATE and encode plugin registrations (i.e., insert the
.plugin(PLUGIN_NAME_RUNTIME_WRAPPER).use(...) block before the template
registration), or alternatively call .before(PLUGIN_NAME_TEMPLATE) (and .before
the LynxEncode/WebEncode plugin names if available) on the chain entry so the
runtime wrapper executes prior to the template/encoding transforms.
…tests for runtime wrapper plugin
PupilTong
force-pushed
the
p/hw/enable-wrapper
branch
from
30a9519 to
5ba7a50
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.changeset/plenty-actors-cheer.md (1)
9-13: Call out all impacted packages and dependency requirements.Per prior reviews,
react-rsbuild-pluginalso requires@lynx-js/rspeedyv0.12.0. Add that to the migration notes and include/add a changeset for any affected published package. If some are private, per team learnings, they don’t need meaningful changesets—verify which are public first.#!/bin/bash # Verify declared dependency on @lynx-js/rspeedy is >= 0.12.0 wherever used. rg -nP --glob '!**/node_modules/**' '"@lynx-js/rspeedy"\s*:\s*"[^\"]+"' -C2 package.json(We used your prior learning about private packages when suggesting this.)
🧹 Nitpick comments (3)
.changeset/plenty-actors-cheer.md (3)
7-7: Fix markdown heading and be explicit about what to upgrade.Avoid bold-as-heading (MD036) and clarify it’s the environment.web payload (v2) that requires a compatible runtime.
-**Breaking Change, you will need to upgrade your web platform version** +## Breaking changes +Upgrade to environment.web payload version 2 (WebEncodePlugin output). Ensure your web runtime supports v2.
5-5: Prefer human-facing summary over commit-style subject.Changeset bodies target end users; drop the
feat(...)prefix.-feat(web-platform): migrate environment.web output format and update tests for runtime wrapper plugin +Migrate environment.web output format and update tests for the runtime wrapper plugin.
9-15: Rewrite for end users; add migration steps and avoid internal jargon.Current text mentions “chunk format”/“IIFE” without guidance. Propose end-user framing with a concise migration guide.
-Before this commit, the environment.web output has its own chunk format. - -After this commit, we migrate those into the format running on iOS/Android. - -The new format has such special features: - -- some global variables, is exported by the IIFE wrapper +### What changed +- `environment.web` now uses the same payload format as iOS/Android. +- The payload is wrapped in a runtime wrapper and includes `version: 2` in the encoded output. + +### Migration +- Upgrade your web runtime/tooling to a version that understands payload v2. +- Update `@lynx-js/react-rsbuild-plugin` to this release. +- Rebuild. No app code changes are expected unless you parse `environment.web` output directly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/plenty-actors-cheer.md(1 hunks)packages/rspeedy/plugin-react/src/entry.ts(1 hunks)packages/rspeedy/plugin-react/test/web.test.ts(2 hunks)packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/webpack/template-webpack-plugin/src/WebEncodePlugin.ts
- packages/rspeedy/plugin-react/test/web.test.ts
- packages/rspeedy/plugin-react/src/entry.ts
🧰 Additional context used
📓 Path-based instructions (1)
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
For contributions, always generate a changeset and commit the resulting markdown file(s)
Files:
.changeset/plenty-actors-cheer.md
🧠 Learnings (1)
📚 Learning: 2025-09-12T09:43:04.847Z
Learnt from: gaoachao
PR: lynx-family/lynx-stack#1736
File: .changeset/spotty-experts-smoke.md:1-3
Timestamp: 2025-09-12T09:43:04.847Z
Learning: In the lynx-family/lynx-stack repository, private packages (marked with "private": true in package.json) like lynx-js/react-transform don't require meaningful changeset entries even when their public APIs change, since they are not published externally and only affect internal development.
Applied to files:
.changeset/plenty-actors-cheer.md
🪛 markdownlint-cli2 (0.17.2)
.changeset/plenty-actors-cheer.md
7-7: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (1)
.changeset/plenty-actors-cheer.md (1)
1-3: Bump changeset to major for a declared breaking changeBody declares a breaking change but this changeset currently bumps @lynx-js/react-rsbuild-plugin as minor — change to major. If other published packages are affected, add separate changeset entries per package.
File: .changeset/plenty-actors-cheer.md (lines 1-3)
--- -"@lynx-js/react-rsbuild-plugin": minor +"@lynx-js/react-rsbuild-plugin": major ---Verification: the provided verification script returned no changed-package results — re-run the script or confirm which published packages changed and their
privateflags.
3 participants
Summary by CodeRabbit
New Features
Tests
Chores
Checklist