Conversation
…ugin
Expose plugin hooks under the new Symbol.for('LynxBundlePlugin') while
keeping backward compat with the old Symbol.for('LynxTemplatePlugin').
Consumer side uses fallback pattern for compatibility with older versions.
🦋 Changeset detectedLatest commit: 19267fc The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
📝 WalkthroughWalkthroughA symbol renaming initiative updates the exported API from LynxTemplatePlugin to LynxBundlePlugin across multiple packages. The changes introduce dual exposure handling for backward compatibility, update configuration logic to resolve the new symbol with fallback to the old name, and modify test suites to reflect the new API surface while maintaining legacy symbol access. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR renames the exposed symbol used for inter-plugin communication from LynxTemplatePlugin to LynxBundlePlugin, updating both the producer (pluginReactLynx) and the consumer (pluginLynxConfig) while attempting to maintain backward compatibility with both old and new symbol names.
Changes:
- Rename the exposed symbol from
LynxTemplatePlugintoLynxBundlePluginin the producer plugin, exposing under both new and old symbol names for backward compat - Update the consumer plugin to try the new symbol first, falling back to the old symbol, with a runtime
incheck to resolve the correct property key - Update tests and inline snapshots to reflect the new naming, and add a backward compat test in the producer
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
packages/rspeedy/plugin-react/src/pluginReactLynx.ts |
Producer: expose under both LynxBundlePlugin and LynxTemplatePlugin symbols with the new key structure |
packages/rspeedy/plugin-config/src/pluginLynxConfig.ts |
Consumer: try new symbol first, fall back to old, use in check to resolve correct property key |
packages/rspeedy/plugin-react/test/expose.test.ts |
Updated test for new symbol name and added backward compat test |
packages/rspeedy/plugin-config/test/pluginLynxConfig.test.ts |
Updated inline snapshots and mock structure for new symbol/key naming |
.changeset/expose-symbol-rename.md |
Changeset for both packages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| api.expose(Symbol.for('LynxBundlePlugin'), exposedLynxBundlePlugin) | ||
| // Backward compat: keep exposing under the old symbol | ||
| api.expose(Symbol.for('LynxTemplatePlugin'), exposedLynxBundlePlugin) |
There was a problem hiding this comment.
The backward compatibility is incomplete for the scenario where a new pluginReactLynx (producer) is used with an old pluginLynxConfig (consumer).
The old consumer code (before this PR) destructured LynxTemplatePlugin from the exposed object:
const { LynxTemplatePlugin: LynxTemplatePluginClass } = exposed
But the new producer exposes { LynxBundlePlugin: ... } under the old Symbol.for('LynxTemplatePlugin') symbol. This means the old consumer would get undefined for LynxTemplatePluginClass, causing a runtime error.
To fix this, the object exposed under the old symbol should also include the old key name. For example, include both keys (LynxBundlePlugin and LynxTemplatePlugin) in the object exposed under the old symbol, or expose a separate object with the old key name under the old symbol.
| test('backward compat: old LynxTemplatePlugin symbol still works', async () => { | ||
| const { pluginReactLynx } = await import('../src/index.js') | ||
|
|
||
| let expose: { LynxBundlePlugin: LynxTemplatePlugin } | undefined | ||
|
|
||
| const rsbuild = await createRspeedy({ | ||
| rspeedyConfig: { | ||
| source: { | ||
| entry: { | ||
| main: new URL('./fixtures/basic.tsx', import.meta.url).pathname, | ||
| }, | ||
| }, | ||
| plugins: [ | ||
| pluginReactLynx(), | ||
| pluginStubRspeedyAPI(), | ||
| { | ||
| name: 'pluginThatUsesOldSymbol', | ||
| setup(api) { | ||
| expose = api.useExposed< | ||
| { LynxBundlePlugin: LynxTemplatePlugin } | ||
| >(Symbol.for('LynxTemplatePlugin')) | ||
| }, | ||
| } as RsbuildPlugin, | ||
| ], | ||
| }, | ||
| }) | ||
|
|
||
| await rsbuild.initConfigs() | ||
| expect(expose).toBeDefined() | ||
| expect(expose!.LynxBundlePlugin).toBeDefined() | ||
| expect(expose!.LynxBundlePlugin.getLynxTemplatePluginHooks).toBeTypeOf( | ||
| 'function', | ||
| ) |
There was a problem hiding this comment.
This backward compatibility test does not accurately simulate what an old consumer would do. An old consumer (pre-rename) would look up Symbol.for('LynxTemplatePlugin') and then access the LynxTemplatePlugin property from the result — not LynxBundlePlugin.
The test should use { LynxTemplatePlugin: LynxTemplatePlugin } as the type and assert expose!.LynxTemplatePlugin is defined and has getLynxTemplatePluginHooks. As written, this test passes but doesn't catch the real backward compatibility issue: the old key LynxTemplatePlugin is missing from the object exposed under the old symbol.
| api.expose(Symbol.for('LynxBundlePlugin'), { | ||
| LynxBundlePlugin: { getLynxTemplatePluginHooks }, |
There was a problem hiding this comment.
Missing test coverage for the backward compatibility path in the consumer. There's no test verifying that pluginLynxConfig works when the producer only exposes under the old Symbol.for('LynxTemplatePlugin') with the old { LynxTemplatePlugin: ... } key structure (i.e., old producer + new consumer scenario). This is the scenario addressed by lines 120-124 and 159-161 of pluginLynxConfig.ts, and it would be valuable to have a test covering it to prevent regressions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rspeedy/plugin-react/src/pluginReactLynx.ts`:
- Around line 392-400: The code currently exposes exposedLynxBundlePlugin under
both Symbol.for('LynxBundlePlugin') and Symbol.for('LynxTemplatePlugin') but the
latter keeps the new payload shape, breaking consumers expecting a
LynxTemplatePlugin property; fix by creating a legacy wrapper object that
includes a LynxTemplatePlugin property (pointing to the same
getLynxTemplatePluginHooks bound function) and use that when calling
api.expose(Symbol.for('LynxTemplatePlugin'), ...); reference
exposedLynxBundlePlugin, LynxTemplatePlugin.getLynxTemplatePluginHooks, and the
api.expose calls when making the change.
In `@packages/rspeedy/plugin-react/test/expose.test.ts`:
- Around line 95-127: The test "backward compat: old LynxTemplatePlugin symbol
still works" currently asserts expose!.LynxBundlePlugin but should assert the
legacy property expose!.LynxTemplatePlugin to prove backward compatibility;
update the assertions that check the exposed property (the variable expose) to
reference LynxTemplatePlugin and verify its getLynxTemplatePluginHooks is a
function, keeping the Symbol.for('LynxTemplatePlugin') usage and existing
pluginReactLynx/imports intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05248147-8796-4b33-95ff-651162b46d63
📒 Files selected for processing (5)
.changeset/expose-symbol-rename.mdpackages/rspeedy/plugin-config/src/pluginLynxConfig.tspackages/rspeedy/plugin-config/test/pluginLynxConfig.test.tspackages/rspeedy/plugin-react/src/pluginReactLynx.tspackages/rspeedy/plugin-react/test/expose.test.ts
| const exposedLynxBundlePlugin = { | ||
| LynxBundlePlugin: { | ||
| getLynxTemplatePluginHooks: LynxTemplatePlugin | ||
| .getLynxTemplatePluginHooks.bind(LynxTemplatePlugin), | ||
| }, | ||
| }) | ||
| } | ||
| api.expose(Symbol.for('LynxBundlePlugin'), exposedLynxBundlePlugin) | ||
| // Backward compat: keep exposing under the old symbol | ||
| api.expose(Symbol.for('LynxTemplatePlugin'), exposedLynxBundlePlugin) |
There was a problem hiding this comment.
Keep the legacy wrapper under Symbol.for('LynxTemplatePlugin').
This preserves the old symbol name, but not the old payload shape. Any consumer still doing api.useExposed(Symbol.for('LynxTemplatePlugin'))?.LynxTemplatePlugin will now get undefined, so the backward-compat contract is broken.
💡 Proposed fix
- const exposedLynxBundlePlugin = {
- LynxBundlePlugin: {
- getLynxTemplatePluginHooks: LynxTemplatePlugin
- .getLynxTemplatePluginHooks.bind(LynxTemplatePlugin),
- },
- }
- api.expose(Symbol.for('LynxBundlePlugin'), exposedLynxBundlePlugin)
- // Backward compat: keep exposing under the old symbol
- api.expose(Symbol.for('LynxTemplatePlugin'), exposedLynxBundlePlugin)
+ const lynxBundlePlugin = {
+ getLynxTemplatePluginHooks: LynxTemplatePlugin
+ .getLynxTemplatePluginHooks.bind(LynxTemplatePlugin),
+ }
+ api.expose(Symbol.for('LynxBundlePlugin'), {
+ LynxBundlePlugin: lynxBundlePlugin,
+ })
+ // Backward compat: keep the old symbol and the old property name.
+ api.expose(Symbol.for('LynxTemplatePlugin'), {
+ LynxTemplatePlugin: lynxBundlePlugin,
+ LynxBundlePlugin: lynxBundlePlugin,
+ })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rspeedy/plugin-react/src/pluginReactLynx.ts` around lines 392 - 400,
The code currently exposes exposedLynxBundlePlugin under both
Symbol.for('LynxBundlePlugin') and Symbol.for('LynxTemplatePlugin') but the
latter keeps the new payload shape, breaking consumers expecting a
LynxTemplatePlugin property; fix by creating a legacy wrapper object that
includes a LynxTemplatePlugin property (pointing to the same
getLynxTemplatePluginHooks bound function) and use that when calling
api.expose(Symbol.for('LynxTemplatePlugin'), ...); reference
exposedLynxBundlePlugin, LynxTemplatePlugin.getLynxTemplatePluginHooks, and the
api.expose calls when making the change.
| test('backward compat: old LynxTemplatePlugin symbol still works', async () => { | ||
| const { pluginReactLynx } = await import('../src/index.js') | ||
|
|
||
| let expose: { LynxBundlePlugin: LynxTemplatePlugin } | undefined | ||
|
|
||
| const rsbuild = await createRspeedy({ | ||
| rspeedyConfig: { | ||
| source: { | ||
| entry: { | ||
| main: new URL('./fixtures/basic.tsx', import.meta.url).pathname, | ||
| }, | ||
| }, | ||
| plugins: [ | ||
| pluginReactLynx(), | ||
| pluginStubRspeedyAPI(), | ||
| { | ||
| name: 'pluginThatUsesOldSymbol', | ||
| setup(api) { | ||
| expose = api.useExposed< | ||
| { LynxBundlePlugin: LynxTemplatePlugin } | ||
| >(Symbol.for('LynxTemplatePlugin')) | ||
| }, | ||
| } as RsbuildPlugin, | ||
| ], | ||
| }, | ||
| }) | ||
|
|
||
| await rsbuild.initConfigs() | ||
| expect(expose).toBeDefined() | ||
| expect(expose!.LynxBundlePlugin).toBeDefined() | ||
| expect(expose!.LynxBundlePlugin.getLynxTemplatePluginHooks).toBeTypeOf( | ||
| 'function', | ||
| ) |
There was a problem hiding this comment.
Assert the legacy property when claiming backward compatibility.
This test only proves that the old symbol resolves; it does not prove that old consumers still work. To verify the legacy contract, the assertion needs to read .LynxTemplatePlugin, not .LynxBundlePlugin.
💡 Proposed fix
- let expose: { LynxBundlePlugin: LynxTemplatePlugin } | undefined
+ let expose:
+ | {
+ LynxTemplatePlugin: LynxTemplatePlugin
+ LynxBundlePlugin?: LynxTemplatePlugin
+ }
+ | undefined
@@
- expect(expose!.LynxBundlePlugin).toBeDefined()
- expect(expose!.LynxBundlePlugin.getLynxTemplatePluginHooks).toBeTypeOf(
+ expect(expose!.LynxTemplatePlugin).toBeDefined()
+ expect(expose!.LynxTemplatePlugin.getLynxTemplatePluginHooks).toBeTypeOf(
'function',
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rspeedy/plugin-react/test/expose.test.ts` around lines 95 - 127, The
test "backward compat: old LynxTemplatePlugin symbol still works" currently
asserts expose!.LynxBundlePlugin but should assert the legacy property
expose!.LynxTemplatePlugin to prove backward compatibility; update the
assertions that check the exposed property (the variable expose) to reference
LynxTemplatePlugin and verify its getLynxTemplatePluginHooks is a function,
keeping the Symbol.for('LynxTemplatePlugin') usage and existing
pluginReactLynx/imports intact.
Merging this PR will improve performance by 44.06%
Performance Changes
Comparing 
Footnotes
|
Web Explorer#8007 Bundle Size — 384.21KiB (0%).19267fc(current) vs c877fbd main#8005(baseline) Bundle metrics
|
| Current #8007 |
Baseline #8005 |
|
|---|---|---|
 Initial JS |
155.31KiB |
155.31KiB |
| Initial CSS |
35.1KiB |
35.1KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
| Modules |
239 |
239 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
2.98% |
2.98% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #8007 |
Baseline #8005 |
|
|---|---|---|
| JS |
253.26KiB |
253.26KiB |
| Other |
95.85KiB |
95.85KiB |
| CSS |
35.1KiB |
35.1KiB |
Bundle analysis report Branch Huxpro/align-terms-expose Project dashboard
Generated by RelativeCI Documentation Report issue
Summary
LynxTemplatePlugintoLynxBundlePluginwith backward compatibilitypluginReactLynx) exposes under both new and old symbol namespluginLynxConfig) uses fallback pattern:useExposed('LynxBundlePlugin') ?? useExposed('LynxTemplatePlugin')Test plan
@lynx-js/react-rsbuild-plugin@lynx-js/react-rsbuild-plugin@lynx-js/config-rsbuild-pluginSummary by CodeRabbit