-
Notifications
You must be signed in to change notification settings - Fork 120
fix(react/runtime): export correct cloneElement
on main-thread
#1508
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 15f8838 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
📝 WalkthroughWalkthroughCentralizes createElement/cloneElement behind environment-aware runtime wrappers, updates runtime exports and Suspense to use them, replaces thread-conditional cloning in DeferredListItem, updates lepus type re-exports to preact/compat, adds a changeset noting a patch release fixing element creation/cloning on main thread, and updates tests/snapshots. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/react/runtime/src/lynx/suspense.ts (2)
18-26
: Prefer typed intrinsic augmentation over ts-expect-error for 'wrapper'.Relying on ts-expect-error hides legitimate type issues. Consider augmenting JSX IntrinsicElements for the lynx runtime so 'wrapper' is recognized during type-checking, eliminating the need for per-callsite suppression.
Add a local type augmentation (new file) to your runtime package:
// packages/react/runtime/src/jsx-intrinsics.d.ts // Ensures our internal host element is recognized by TS in Preact JSX. declare namespace JSX { interface IntrinsicElements { wrapper: any; } }After this, you can remove the ts-expect-error comments here.
28-38
: Potential hot-path O(n) removal; consider a Set if this list grows.
indexOf
+splice
is O(n). IfglobalBackgroundSnapshotInstancesToRemove
tends to be large or modified frequently under load, switching it to a Set (with add/delete) will reduce churn and simplify the logic.packages/react/runtime/src/cloneElement.ts (1)
9-15
: Add a short docblock to explain the split-brain behavior.A brief note helps future maintainers understand why main-thread delegates to lepus (legacy lazy-bundle compatibility) while background uses preact/compat.
Apply this diff:
-export const cloneElement: typeof import('preact/compat').cloneElement = /*#__PURE__*/ (() => { +// Environment-aware cloneElement selection: +// - main-thread: delegate to lepus to preserve legacy lazy-bundle semantics +// - background: use Preact compat implementation +export const cloneElement: typeof import('preact/compat').cloneElement = /*#__PURE__*/ (() => { if (__BACKGROUND__) { return preactCloneElement; } else { return mainThreadCloneElement; } })();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/tall-seals-notice.md
(1 hunks)packages/react/components/src/DeferredListItem.tsx
(2 hunks)packages/react/runtime/lepus/index.d.ts
(1 hunks)packages/react/runtime/src/cloneElement.ts
(1 hunks)packages/react/runtime/src/createElement.ts
(1 hunks)packages/react/runtime/src/index.ts
(2 hunks)packages/react/runtime/src/lynx/suspense.ts
(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-18T04:27:18.291Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1238
File: packages/react/runtime/src/debug/component-stack.ts:70-90
Timestamp: 2025-07-18T04:27:18.291Z
Learning: The component-stack.ts file in packages/react/runtime/src/debug/component-stack.ts is a direct fork from https://github.com/preactjs/preact/blob/main/debug/src/component-stack.js. The team prefers to keep it aligned with the upstream Preact version and may contribute improvements back to Preact in the future.
Applied to files:
packages/react/runtime/lepus/index.d.ts
packages/react/runtime/src/cloneElement.ts
📚 Learning: 2025-07-22T09:26:16.722Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:26:16.722Z
Learning: In the lynx-family/lynx-stack repository, CI checks require changesets when files matching the pattern "src/**" are modified (as configured in .changeset/config.json). For internal changes that don't need meaningful changesets, an empty changeset file is used to satisfy the CI requirement while not generating any release notes.
Applied to files:
.changeset/tall-seals-notice.md
📚 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/tall-seals-notice.md
📚 Learning: 2025-08-07T04:00:59.627Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1454
File: pnpm-workspace.yaml:46-46
Timestamp: 2025-08-07T04:00:59.627Z
Learning: In the lynx-family/lynx-stack repository, the webpack patch (patches/webpack5.101.0.patch) was created to fix issues with webpack5.99.9 but only takes effect on webpack5.100.0 and later versions. The patchedDependencies entry should use "webpack@^5.100.0" to ensure the patch applies to the correct version range.
Applied to files:
.changeset/tall-seals-notice.md
🧬 Code Graph Analysis (4)
packages/react/runtime/src/createElement.ts (2)
packages/react/runtime/lepus/index.d.ts (1)
createElement
(4-4)packages/react/runtime/src/index.ts (1)
createElement
(88-88)
packages/react/runtime/src/cloneElement.ts (2)
packages/react/runtime/lepus/index.d.ts (1)
cloneElement
(4-4)packages/react/runtime/src/index.ts (1)
cloneElement
(89-89)
packages/react/components/src/DeferredListItem.tsx (2)
packages/react/runtime/src/cloneElement.ts (1)
cloneElement
(9-15)packages/react/runtime/lepus/index.js (1)
cloneElement
(54-87)
packages/react/runtime/src/lynx/suspense.ts (2)
packages/react/runtime/src/backgroundSnapshot.ts (1)
BackgroundSnapshotInstance
(35-361)packages/react/runtime/src/createElement.ts (1)
createElement
(8-14)
🔇 Additional comments (12)
.changeset/tall-seals-notice.md (1)
1-6
: LGTM! Changeset properly documents the fix.The changeset correctly identifies this as a patch release for
@lynx-js/react
and provides a clear description of the fix forcloneElement
andcreateElement
on the main thread. Based on the retrieved learnings, this represents a meaningful bugfix that warrants a changeset entry.packages/react/runtime/lepus/index.d.ts (1)
4-4
: Confirm Preact/compatcreateElement
&cloneElement
Type Compatibility
We couldn’t locate local.d.ts
declarations for these APIs—please manually verify that re-exporting frompreact/compat
preserves React’s signatures to avoid breaking changes:• File under review:
– packages/react/runtime/lepus/index.d.ts:4 (export { cloneElement, createElement } from 'preact/compat';
)
• Inspect Preact’s shipped types (e.g.node_modules/preact/compat/dist/compat.d.ts
) forcreateElement
/cloneElement
overloads
• Compare against React’s definitions (e.g.node_modules/@types/react/index.d.ts
) for matching generics, parameters, and return typesEnsure any discrepancies are resolved (via type overrides or updated imports) before merging.
packages/react/components/src/DeferredListItem.tsx (2)
7-7
: LGTM! Centralized cloneElement import simplifies the code.The change from thread-dependent imports to a single
cloneElement
import from@lynx-js/react
eliminates complexity while maintaining the same functionality through the new environment-aware wrapper inpackages/react/runtime/src/cloneElement.ts
.
50-53
: LGTM! Simplified cloneElement usage removes conditional complexity.The elimination of the
__cloneElement
wrapper and__MAIN_THREAD__
gating simplifies the code while maintaining the same functionality. The centralizedcloneElement
from the runtime wrapper handles the environment detection automatically.packages/react/runtime/src/index.ts (2)
21-22
: LGTM! Local wrapper imports provide environment-aware functionality.The imports from local wrapper modules (
./cloneElement.js
and./createElement.js
) replace the direct preact/compat imports, enabling the runtime to automatically select the appropriate implementation based on the execution environment.
75-75
: LGTM! Consistent export of cloneElement through both default and named exports.Adding
cloneElement
to both the default export object and named exports maintains API consistency and ensures the centralized implementation is accessible through all expected paths.Also applies to: 89-89
packages/react/runtime/src/createElement.ts (3)
4-6
: LGTM! Clean import structure for environment-specific implementations.The imports clearly separate the preact/compat implementation from the main-thread lepus implementation, setting up the environment-aware selection pattern.
8-14
: LGTM! Environment-aware createElement wrapper with proper type safety.The IIFE pattern provides clean runtime selection based on the
__BACKGROUND__
flag, and the type annotation ensures compatibility with preact/compat's createElement signature. This mirrors the same pattern used in thecloneElement
wrapper, providing consistency across the runtime.
8-14
: No action needed:__BACKGROUND__
usage is consistent
We’ve confirmed that:
- The flag is declared in runtime/types/types.d.ts, react/docs.d.ts, and test-environment index.ts
- Runtime modules (createElement, cloneElement, root.ts, lynx.ts, debug/profile.ts, etc.) all guard on
__BACKGROUND__
- Tests and the webpack-plugin loader define and stub
__BACKGROUND__
appropriatelyAll environment-aware code handles
__BACKGROUND__
as expected.packages/react/runtime/src/lynx/suspense.ts (2)
6-11
: Good consolidation: centralized createElement and compat Suspense import.Replacing thread-gated imports with the unified createElement wrapper keeps this module consistent with the rest of runtime and removes conditional code paths. No functional regressions visible.
40-41
: LGTM: using the wrapper for Suspense instantiation is consistent.Consistent with the new runtime patterns; fallback and children are composed via the same factory.
packages/react/runtime/src/cloneElement.ts (1)
9-15
: LGTM: environment-aware wrapper aligns with createElement.ts.The PURE IIFE pattern and BACKGROUND gating match the createElement wrapper and allow static branch pruning in builds.
CodSpeed Performance ReportMerging #1508 will not alter performanceComparing Summary
|
Web Explorer#4282 Bundle Size — 348.12KiB (0%).15f8838(current) vs 51a0b19 main#4269(baseline) Bundle metrics
Bundle size by type
|
Current #4282 |
Baseline #4269 |
|
---|---|---|
233.32KiB |
233.32KiB |
|
82.95KiB |
82.95KiB |
|
31.84KiB |
31.84KiB |
Bundle analysis report Branch colinaaa:colin/0812/clone-elemen... Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#4289 Bundle Size — 237.04KiB (~+0.01%).15f8838(current) vs 51a0b19 main#4276(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch colinaaa:colin/0812/clone-elemen... Project dashboard Generated by RelativeCI Documentation Report issue |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/react/runtime/src/cloneElement.ts (1)
8-8
: Double-check lepus.cloneElement forwards variadic childrenWe’ve previously flagged lepus’ implementation only accepting a single
children
arg. Please verify it now uses...children
and forwards them.#!/bin/bash set -euo pipefail echo "cloneElement signature in lepus runtime:" rg -nP -C2 'export\s+function\s+cloneElement\s*\(' packages/react/runtime/lepus/index.js || true echo echo "Look for rest-children usage and forwarding:" rg -nP -C2 '\.\.\.children|cloneElement\([^)]*,[^)]*,\s*\.\.\.children' packages/react/runtime/lepus/index.js || true
🧹 Nitpick comments (4)
packages/react/runtime/__test__/lifecycle/reload.test.jsx (1)
126-126
: Inline snapshot token/id shifts are expected; consider reducing brittlenessThe updated numeric tokens (e.g., -2/-3/-9, id progressions) in these sections match the new patch encoding and hydration sequencing. Tests remain readable, but are fragile to internal id churn.
You could parse the data payload and assert structure/semantics without hard-coding every id. Example pattern for one assertion:
const payload = lynx.getNativeApp().callLepusMethod.mock.calls[0][1]; const parsed = JSON.parse(payload.data); // Assert the essential shape, not exact ids: expect(parsed).toEqual({ patchList: [ expect.objectContaining({ id: expect.any(Number), snapshotPatch: expect.arrayContaining([expect.anything()]), }), ], }); expect(payload.patchOptions.reloadVersion).toBeGreaterThanOrEqual(0);Also applies to: 308-309, 538-538, 653-653, 809-810, 1515-1520, 1716-1718, 1876-1888
packages/react/runtime/src/cloneElement.ts (1)
12-13
: Align internal signature with preact and drop redundant children init
- Match preact’s signature for easier local reasoning.
children ??= []
is redundant for a rest parameter.Apply:
- return function<P>(vnode: VNode, props?: Attributes & P, ...children: ComponentChildren[]) { - children ??= []; + return function<P>(vnode: VNode<P>, props?: (Attributes & P) | null, ...children: ComponentChildren[]) {packages/react/runtime/__test__/preact.test.jsx (1)
650-672
: Prefer static import with alias over dynamic import for cloneElement wrapperDynamic import isn’t necessary here and makes symbol origins harder to follow given the top-level
cloneElement
from preact. Use a static alias.Apply:
- it('cloneElement', async function() { - const { cloneElement } = await import('../src/cloneElement'); + it('cloneElement', async function() {And update the call site:
- render(cloneElement(jsx, { bar: 'baz' }), scratch); + render(runtimeCloneElement(jsx, { bar: 'baz' }), scratch);Add this top-level import (outside this hunk):
import { cloneElement as runtimeCloneElement } from '../src/cloneElement';packages/react/runtime/src/createElement.ts (1)
12-13
: Match preact’s signature and remove redundant children init
- Allow
props
to beundefined|null
to mirror preact.children ??= []
is unnecessary for a rest param.Apply:
- return function<P>(type: ComponentType<P>, props: Attributes & P, ...children: ComponentChildren[]) { - children ??= []; + return function<P>(type: ComponentType<P>, props?: (Attributes & P) | null, ...children: ComponentChildren[]) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
packages/react/runtime/__test__/lifecycle/reload.test.jsx
(11 hunks)packages/react/runtime/__test__/lifecycle/reloadMT.jsx
(0 hunks)packages/react/runtime/__test__/list.test.jsx
(2 hunks)packages/react/runtime/__test__/preact.test.jsx
(6 hunks)packages/react/runtime/__test__/renderToOpcodes.test.jsx
(0 hunks)packages/react/runtime/__test__/snapshot/workletRefMT.jsx
(0 hunks)packages/react/runtime/__test__/ssr.test.jsx
(0 hunks)packages/react/runtime/src/cloneElement.ts
(1 hunks)packages/react/runtime/src/createElement.ts
(1 hunks)
💤 Files with no reviewable changes (4)
- packages/react/runtime/test/renderToOpcodes.test.jsx
- packages/react/runtime/test/snapshot/workletRefMT.jsx
- packages/react/runtime/test/lifecycle/reloadMT.jsx
- packages/react/runtime/test/ssr.test.jsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1500
File: .changeset/curvy-dragons-appear.md:1-5
Timestamp: 2025-08-13T11:50:58.069Z
Learning: In the lynx-family/lynx-stack repository, all packages under packages/react/ should use "lynx-js/react" in changesets since they are all published together under the same package name, regardless of their individual package.json names. This includes packages/react/testing-library and other sub-packages.
📚 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/react/testing-library/src/vitest.config.js` is source code for the testing library that gets exported for users, not a test configuration that should be included in the main vitest projects array.
Applied to files:
packages/react/runtime/__test__/list.test.jsx
📚 Learning: 2025-07-18T04:27:18.291Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1238
File: packages/react/runtime/src/debug/component-stack.ts:70-90
Timestamp: 2025-07-18T04:27:18.291Z
Learning: The component-stack.ts file in packages/react/runtime/src/debug/component-stack.ts is a direct fork from https://github.com/preactjs/preact/blob/main/debug/src/component-stack.js. The team prefers to keep it aligned with the upstream Preact version and may contribute improvements back to Preact in the future.
Applied to files:
packages/react/runtime/src/cloneElement.ts
📚 Learning: 2025-08-13T09:20:00.936Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1502
File: packages/react/testing-library/types/entry.d.ts:71-71
Timestamp: 2025-08-13T09:20:00.936Z
Learning: In lynx-js/react testing library, wrapper components must have children as a required prop because they are always called with `h(WrapperComponent, null, innerElement)` where innerElement is passed as children. The type `React.JSXElementConstructor<{ children: React.ReactNode }>` correctly requires children to be mandatory.
Applied to files:
packages/react/runtime/src/cloneElement.ts
🧬 Code Graph Analysis (3)
packages/react/runtime/src/createElement.ts (6)
packages/react/runtime/lepus/index.d.ts (1)
createElement
(4-4)packages/react/runtime/src/index.ts (1)
createElement
(88-88)packages/webpack/template-webpack-plugin/test/cases/assets/production/rspack.config.js (1)
process
(34-34)packages/react/types/react.d.ts (2)
ComponentType
(27-27)Attributes
(7-7)packages/web-platform/web-tests/tests/react/basic-mts-mainthread-nested-ref/index.jsx (1)
props
(7-7)packages/web-platform/offscreen-document/src/webworker/OffscreenElement.ts (1)
children
(103-105)
packages/react/runtime/__test__/preact.test.jsx (3)
packages/react/runtime/src/backgroundSnapshot.ts (1)
BackgroundSnapshotInstance
(35-361)packages/react/runtime/src/createElement.ts (1)
createElement
(10-28)packages/react/runtime/src/cloneElement.ts (1)
cloneElement
(10-29)
packages/react/runtime/src/cloneElement.ts (6)
packages/react/runtime/lepus/index.d.ts (1)
cloneElement
(4-4)packages/react/runtime/src/index.ts (1)
cloneElement
(89-89)packages/webpack/template-webpack-plugin/test/cases/assets/production/rspack.config.js (1)
process
(34-34)packages/web-platform/web-tests/tests/react/basic-mts-mainthread-nested-ref/index.jsx (1)
props
(7-7)packages/react/types/react.d.ts (1)
Attributes
(7-7)packages/web-platform/offscreen-document/src/webworker/OffscreenElement.ts (1)
children
(103-105)
🔇 Additional comments (5)
packages/react/runtime/__test__/list.test.jsx (1)
3503-3515
: Approve: snapshot handlerName index update is correct — no stale expectations foundVerified: the test snapshot now uses -3/-2 and no occurrences of the old -6/-4 handlerName values remain.
- packages/react/runtime/test/list.test.jsx — line 3504: "handlerName": "-3:__extraProps:onComponentAtIndex"
- packages/react/runtime/test/list.test.jsx — line 3513: "handlerName": "-2:__extraProps:onRecycleComponent"
- Repo-wide search: no matches for "-6:__extraProps:onComponentAtIndex" or "-4:__extraProps:onRecycleComponent" in packages.
packages/react/runtime/src/cloneElement.ts (1)
10-29
: Good centralization of cloneElement behind an env-aware wrapperThe NODE_ENV-aware indirection and per-call branching under tests look correct. Export type matches preact/compat, keeping callers consistent.
packages/react/runtime/__test__/preact.test.jsx (2)
629-648
: Nice background-path coverage for createElement wrapperGood use of the wrapper and snapshot stabilization to validate the background implementation selection.
696-716
: Snapshot id shifts after inserting new tests look correctThe card ids and references bumped as expected (26/27 → 28/29). Nothing else functionally changed in the structure.
packages/react/runtime/src/createElement.ts (1)
10-28
: Environment-aware createElement wrapper looks solidThe per-call selection in tests and static selection otherwise mirrors cloneElement; export type matches preact/compat.
import { createElement as preactCreateElement } from 'preact/compat'; | ||
import type { ComponentType } from 'preact/compat'; | ||
|
||
import { createElement as mainThreadCreateElement } from '../lepus/index.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Verify lepus.createElement also accepts and forwards variadic children
To maintain parity, ensure createElement
in lepus uses ...children
and forwards them.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "createElement signature in lepus runtime:"
rg -nP -C2 'export\s+function\s+createElement\s*\(' packages/react/runtime/lepus/index.js || true
echo
echo "Look for rest-children usage and forwarding:"
rg -nP -C2 '\.\.\.children|createElement\([^)]*,[^)]*,\s*\.\.\.children' packages/react/runtime/lepus/index.js || true
Length of output: 562
Action: Make lepus.createElement accept and forward variadic children
Found: packages/react/runtime/lepus/index.js declares
export function createElement(type, props, children) { … } (no variadic forwarding found).
What to change:
- Update signature to accept rest children and forward them to the underlying VNode creation/calls.
- File to modify:
- packages/react/runtime/lepus/index.js — change the function signature and ensure any internal calls use ...children.
Suggested diff:
- packages/react/runtime/lepus/index.js
- export function createElement(type, props, children) {
- export function createElement(type, props, ...children) {
🤖 Prompt for AI Agents
packages/react/runtime/lepus/index.js (createElement export) — The createElement
currently takes (type, props, children); change its signature to accept rest
children (type, props, ...children) and update any internal calls that build or
forward VNodes so they use ...children when invoking downstream creation
functions or constructing children arrays; ensure places that assumed a single
children value handle an array of children or spread appropriately and preserve
existing behavior for null/undefined children.
Summary by CodeRabbit
We didn't remove the
@lynx-js/react/lepus
exports to make compatible with the lazy bundles that are built with legacy version of@lynx-js/react
.We should also do the same thing to
@lynx-js/react/jsx-runtime
, maybe in later PRs.Checklist