refactor(react): move runtime hooks out of snapshot#2529
refactor(react): move runtime hooks out of snapshot#2529HuJean merged 4 commits intolynx-family:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 3baf3cf The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
📝 WalkthroughWalkthroughThis PR refactors the React runtime by migrating hooks from snapshot-specific modules to a new core module hierarchy. Shared utilities (profiling, render constants, component stack) are consolidated into a shared directory. Package exports, test files, and internal imports are updated to reference the new module structure throughout the codebase. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Merging this PR will degrade performance by 8.26%
Performance Changes
Comparing Footnotes
|
React Example#7656 Bundle Size — 225.43KiB (+0.02%).3baf3cf(current) vs f6d7177 main#7646(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:wt/runtime-core-shared-ma... Project dashboard Generated by RelativeCI Documentation Report issue |
Web Explorer#9229 Bundle Size — 900.02KiB (0%).3baf3cf(current) vs f6d7177 main#9219(baseline) Bundle metrics
Bundle size by type
|
| Current #9229 |
Baseline #9219 |
|
|---|---|---|
495.88KiB |
495.88KiB |
|
401.92KiB |
401.92KiB |
|
2.22KiB |
2.22KiB |
Bundle analysis report Branch Yradex:wt/runtime-core-shared-ma... Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#788 Bundle Size — 196.58KiB (+0.02%).3baf3cf(current) vs f6d7177 main#778(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:wt/runtime-core-shared-ma... Project dashboard Generated by RelativeCI Documentation Report issue |
React External#772 Bundle Size — 680.41KiB (+0.02%).3baf3cf(current) vs f6d7177 main#763(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:wt/runtime-core-shared-ma... Project dashboard Generated by RelativeCI Documentation Report issue |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/react/runtime/src/shared/profile.ts (1)
5-8: Optional: unify the wrapper pattern across the three exports.
profileStartandprofileEndstill use the trailingas typeof lynx.performance.profileXcast on a double-IIFE pattern, whileprofileFlowIdnow uses a leading type annotation on a single IIFE. Both work, but the inconsistency is a minor readability nit; consider applying the same shape to all three.Also, inlining
noop/noopFlowIdhere (rather than importingnoopfrom../utils.js) is the right call given the new layering rules —shared/should not depend on runtime-root modules — so this duplication is intentional and acceptable.♻️ Example: align profileStart/profileEnd with the profileFlowId pattern
-export const profileStart = /* `@__PURE__` */ ((() => { - let p; - if (!(p = lynx.performance) || typeof p.profileStart !== 'function') { - return noop; - } - return p.profileStart.bind(p); -})()) as typeof lynx.performance.profileStart; - -export const profileEnd = /* `@__PURE__` */ ((() => { - let p; - if (!(p = lynx.performance) || typeof p.profileEnd !== 'function') { - return noop; - } - return p.profileEnd.bind(p); -})()) as typeof lynx.performance.profileEnd; +export const profileStart: typeof lynx.performance.profileStart = /* `@__PURE__` */ (() => { + let p; + if (!(p = lynx.performance) || typeof p.profileStart !== 'function') { + return noop; + } + return p.profileStart.bind(p); +})(); + +export const profileEnd: typeof lynx.performance.profileEnd = /* `@__PURE__` */ (() => { + let p; + if (!(p = lynx.performance) || typeof p.profileEnd !== 'function') { + return noop; + } + return p.profileEnd.bind(p); +})();Also applies to: 30-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/shared/profile.ts` around lines 5 - 8, Unify the wrapper pattern by changing profileStart and profileEnd to the same single-IIFE leading type-annotation style used by profileFlowId: declare const profileStart: typeof lynx.performance.profileStart = (() => { ... })(); and const profileEnd: typeof lynx.performance.profileEnd = (() => { ... })(); remove the trailing "as typeof ..." casts and the double-IIFE pattern, keep the local noop and noopFlowId definitions as-is, and ensure each IIFE returns the appropriate function implementation matching lynx.performance.profileX signatures.packages/react/runtime/src/core/hooks/mainThreadImpl.ts (1)
44-47: Nit: prefer locals over module-levelletfor captured handlers.
oldBeforeDiff/oldBeforeRender/oldAfterDiff/oldRootare only read from closures created insideinstallMainThreadHooksand never reassigned after install. Hoisting them to module scope just widens visibility and keeps stale references alive. Consider declaring them asconstinside the function:♻️ Proposed cleanup
-let hooksInstalled = false; -let oldBeforeDiff: ((vnode: any) => void) | undefined; -let oldBeforeRender: ((vnode: any) => void) | undefined; -let oldAfterDiff: ((vnode: any) => void) | undefined; -let oldRoot: ((vnode: any, parentDom: any) => void) | undefined; - -function installMainThreadHooks(): void { - if (hooksInstalled) { - return; - } - hooksInstalled = true; - oldBeforeDiff = options[DIFF]; - oldBeforeRender = options[RENDER]; - oldAfterDiff = options[DIFFED]; - oldRoot = options[ROOT]; +let hooksInstalled = false; + +function installMainThreadHooks(): void { + if (hooksInstalled) { + return; + } + hooksInstalled = true; + const oldBeforeDiff = options[DIFF]; + const oldBeforeRender = options[RENDER]; + const oldAfterDiff = options[DIFFED]; + const oldRoot = options[ROOT];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/core/hooks/mainThreadImpl.ts` around lines 44 - 47, The module-level mutable bindings oldBeforeDiff, oldBeforeRender, oldAfterDiff, and oldRoot are only captured by closures in installMainThreadHooks and never reassigned, so move them inside installMainThreadHooks and declare them as const locals to limit scope and avoid keeping stale module-level references; update any closures inside installMainThreadHooks to reference the new local consts (preserving their initialization from the original preact hooks) and remove the module-level declarations.packages/react/runtime/__test__/snapshot/lifecycle/updateGlobalProps.test.jsx (1)
30-38: Optional: hoist the dynamic imports out ofrenderWithCurrentPreact.Each call dynamically re-imports
preactand./document. With multiple background-render call sites (5 in this file) the helper does redundantPromise.allwork and re-runssetupBackgroundDocument()on every render. Consider doing the imports once at module top-level (or memoizing inside the helper) so each render call only does the assignment + render.♻️ Sketch
-async function renderWithCurrentPreact(element, root) { - const [preact, { document, setupBackgroundDocument }] = await Promise.all([ - import('preact'), - import('../../../src/document'), - ]); - setupBackgroundDocument(); - preact.options.document = document; - preact.render(element, root); -} +let preactRenderSetup; +async function renderWithCurrentPreact(element, root) { + preactRenderSetup ??= (async () => { + const [preact, { document, setupBackgroundDocument }] = await Promise.all([ + import('preact'), + import('../../../src/document'), + ]); + setupBackgroundDocument(); + preact.options.document = document; + return preact; + })(); + const preact = await preactRenderSetup; + preact.render(element, root); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/__test__/snapshot/lifecycle/updateGlobalProps.test.jsx` around lines 30 - 38, The helper renderWithCurrentPreact repeatedly re-imports preact and ../../../src/document and re-runs setupBackgroundDocument on each call; hoist the dynamic imports out of renderWithCurrentPreact (or memoize them) so the module does the Promise.all once at top-level: import preact and the document module (or create cached vars for them), call setupBackgroundDocument once, then have renderWithCurrentPreact only perform preact.options.document = document and preact.render(element, root); ensure you reference the existing symbols renderWithCurrentPreact, setupBackgroundDocument, preact.options.document, and the dynamic imports import('preact') / import('../../../src/document') when moving the logic.
🤖 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/react/package.json`:
- Around line 62-65: Create a new type declaration (e.g.,
runtime/lib/core/hooks/mainThread.d.ts) that reflects the noop implementations
for useEffect, useLayoutEffect, and useImperativeHandle (the no-op stubs in
mainThreadImpl.ts lines ~213-215) by typing them as returning void or adding
JSDoc deprecation/runtime-warning comments; then update the package.json exports
entry for "./lepus/hooks" (currently pointing to
runtime/lib/core/hooks/react.d.ts) to point its "types" to the new
mainThread.d.ts and also update the corresponding typesVersions mapping to
reference the new file so consumers of `@lynx-js/react/lepus/hooks` get accurate
typings for the noop runtime.
In
`@packages/react/runtime/__test__/snapshot/lifecycle/updateGlobalProps.test.jsx`:
- Line 386: The tests import '../../../src/lynx-api?global-props-event' and rely
on distinct module instances, but Vitest ignores query params and caches the
module (so the shared _GlobalProps singleton gets the first-set
__GLOBAL_PROPS_MODE__); fix by calling vi.resetModules() before each test (or
immediately before the import) so the module cache is cleared and each import
re-evaluates a fresh _GlobalProps instance—add vi.resetModules() in the
beforeEach or directly above the import that references
'../../../src/lynx-api?global-props-event' and similarly for
'?global-props-provider-event'.
---
Nitpick comments:
In
`@packages/react/runtime/__test__/snapshot/lifecycle/updateGlobalProps.test.jsx`:
- Around line 30-38: The helper renderWithCurrentPreact repeatedly re-imports
preact and ../../../src/document and re-runs setupBackgroundDocument on each
call; hoist the dynamic imports out of renderWithCurrentPreact (or memoize them)
so the module does the Promise.all once at top-level: import preact and the
document module (or create cached vars for them), call setupBackgroundDocument
once, then have renderWithCurrentPreact only perform preact.options.document =
document and preact.render(element, root); ensure you reference the existing
symbols renderWithCurrentPreact, setupBackgroundDocument,
preact.options.document, and the dynamic imports import('preact') /
import('../../../src/document') when moving the logic.
In `@packages/react/runtime/src/core/hooks/mainThreadImpl.ts`:
- Around line 44-47: The module-level mutable bindings oldBeforeDiff,
oldBeforeRender, oldAfterDiff, and oldRoot are only captured by closures in
installMainThreadHooks and never reassigned, so move them inside
installMainThreadHooks and declare them as const locals to limit scope and avoid
keeping stale module-level references; update any closures inside
installMainThreadHooks to reference the new local consts (preserving their
initialization from the original preact hooks) and remove the module-level
declarations.
In `@packages/react/runtime/src/shared/profile.ts`:
- Around line 5-8: Unify the wrapper pattern by changing profileStart and
profileEnd to the same single-IIFE leading type-annotation style used by
profileFlowId: declare const profileStart: typeof lynx.performance.profileStart
= (() => { ... })(); and const profileEnd: typeof lynx.performance.profileEnd =
(() => { ... })(); remove the trailing "as typeof ..." casts and the double-IIFE
pattern, keep the local noop and noopFlowId definitions as-is, and ensure each
IIFE returns the appropriate function implementation matching
lynx.performance.profileX signatures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28391a48-3602-49ba-ab00-197ccb00c310
📒 Files selected for processing (52)
.changeset/plain-lions-hope.mdpackages/react/package.jsonpackages/react/runtime/__test__/core/hooks/mainThread.test.tsxpackages/react/runtime/__test__/core/hooks/useLynxGlobalEventListener.test.jsxpackages/react/runtime/__test__/guardrails/snapshot-containment.test.tspackages/react/runtime/__test__/shared/profile.test.tspackages/react/runtime/__test__/snapshot/debug/backgroundSnapshot-profile.test.jsxpackages/react/runtime/__test__/snapshot/debug/profile.test.jsxpackages/react/runtime/__test__/snapshot/debug/react-hooks-profile.test.jsxpackages/react/runtime/__test__/snapshot/debug/vnodeSource.test.tspackages/react/runtime/__test__/snapshot/event.test.jsxpackages/react/runtime/__test__/snapshot/lifecycle.test.jsxpackages/react/runtime/__test__/snapshot/lifecycle/destroy.test.jsxpackages/react/runtime/__test__/snapshot/lifecycle/updateGlobalProps.test.jsxpackages/react/runtime/src/core/hooks/mainThread.tspackages/react/runtime/src/core/hooks/mainThreadImpl.tspackages/react/runtime/src/core/hooks/react.tspackages/react/runtime/src/core/hooks/useLynxGlobalEventListener.tspackages/react/runtime/src/index.tspackages/react/runtime/src/internal.tspackages/react/runtime/src/lynx-api.tspackages/react/runtime/src/lynx.tspackages/react/runtime/src/shared/component-stack.tspackages/react/runtime/src/shared/profile.tspackages/react/runtime/src/shared/render-constants.tspackages/react/runtime/src/snapshot/alog/elementPAPICall.tspackages/react/runtime/src/snapshot/alog/render.tspackages/react/runtime/src/snapshot/compat/initData.tspackages/react/runtime/src/snapshot/debug/profileHooks.tspackages/react/runtime/src/snapshot/debug/vnodeSource.tspackages/react/runtime/src/snapshot/legacy-react-runtime/index.tspackages/react/runtime/src/snapshot/lifecycle/destroy.tspackages/react/runtime/src/snapshot/lifecycle/event/jsReady.tspackages/react/runtime/src/snapshot/lifecycle/isRendering.tspackages/react/runtime/src/snapshot/lifecycle/patch/commit.tspackages/react/runtime/src/snapshot/lifecycle/reload.tspackages/react/runtime/src/snapshot/lifecycle/render.tspackages/react/runtime/src/snapshot/list/listUpdateInfo.tspackages/react/runtime/src/snapshot/lynx/component.tspackages/react/runtime/src/snapshot/lynx/env.tspackages/react/runtime/src/snapshot/lynx/performance.tspackages/react/runtime/src/snapshot/lynx/runWithForce.tspackages/react/runtime/src/snapshot/lynx/tt.tspackages/react/runtime/src/snapshot/renderToOpcodes/hydrate.tspackages/react/runtime/src/snapshot/renderToOpcodes/index.tspackages/react/runtime/src/snapshot/renderToOpcodes/opcodes.tspackages/react/runtime/src/snapshot/snapshot/backgroundSnapshot.tspackages/react/runtime/src/snapshot/worklet/ref/workletRef.tspackages/react/runtime/src/utils.tspackages/react/runtime/vitest.config.tspackages/react/types/react.docs.d.tspackages/rspeedy/plugin-react-alias/test/index.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/react/runtime/__test__/snapshot/lifecycle/updateGlobalProps.event.test.jsx`:
- Around line 125-139: Update the test's description string in the it(...) block
so it correctly reflects what's being exercised: replace the phrase mentioning
useGlobalProps with GlobalPropsConsumer. Locate the it(...) test that defines
Comp and imports GlobalPropsProvider and GlobalPropsConsumer (symbols:
GlobalPropsProvider, GlobalPropsConsumer, Comp) and change the description to
say it tests GlobalPropsProvider and GlobalPropsConsumer to keep the message
consistent with the test body.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0686b61-9c4f-40ae-aec6-8bbe94bd4085
📒 Files selected for processing (4)
packages/react/runtime/__test__/core/hooks/mainThread.test.tsxpackages/react/runtime/__test__/snapshot/lifecycle/updateGlobalProps.event.test.jsxpackages/react/runtime/__test__/snapshot/lifecycle/updateGlobalProps.test.jsxpackages/react/runtime/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react/runtime/test/core/hooks/mainThread.test.tsx
- packages/react/runtime/test/snapshot/lifecycle/updateGlobalProps.test.jsx
Summary by CodeRabbit
Chores
Refactor
Overview
This refactors runtime-owned ReactLynx hooks and supporting Preact integration helpers out of the Snapshot directory so Snapshot no longer appears to own behavior that is shared by multiple runtime backends.
The public imports stay the same, but their package export targets now resolve through
runtime/lib/core/hooks/*instead ofruntime/lib/snapshot/hooks/*.Key Points
src/core/hooks, making hooks a runtime core API surface rather than a Snapshot backend detail.src/shared, including render constants, component stack tracking, and profiling bridge helpers used by hooks and Snapshot debug/runtime modules.coreandsharedmay be consumed by runtime backends, while preventing those directories from depending back on Snapshot or Element Template code.Compatibility / Boundaries
Public package entrypoints are preserved:
Only the internal export targets change:
corenow owns ReactLynx runtime semantics such as hooks.sharedcontains lower-level runtime support used across modules, and must remain backend-neutral.Checklist