feat: remove options.document and reuse vnode as dom#1728
feat: remove options.document and reuse vnode as dom#1728upupming wants to merge 21 commits intolynx-family:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 6d668a8 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 |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRefactors background rendering from BackgroundSnapshotInstance to a DOM-like BackgroundDOM with a new setupDom hook. Removes the document adapter, updates hydrate and cleanup paths, adjusts internal exports/types, switches tests and benchmarks to options.setupDom/vnode.setAttribute, updates package exports and dependencies, and revises testing utilities and global setups. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches and finishing touches and finishing touches and finishing touches and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
upupming
force-pushed
the
feat/simple-bsi
branch
from
39942d8 to
8081585
Compare
Web Explorer#8265 Bundle Size — 385.21KiB (0%).0b1076a(current) vs 726f600 main#8237(baseline) Bundle metrics
|
| Current #8265 |
Baseline #8237 |
|
|---|---|---|
 Initial JS |
155.59KiB |
155.59KiB |
| Initial CSS |
35.1KiB |
35.1KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
| Modules |
238 |
238 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
2.97% |
2.97% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #8265 |
Baseline #8237 |
|
|---|---|---|
| JS |
254.26KiB |
254.26KiB |
| Other |
95.85KiB |
95.85KiB |
| CSS |
35.1KiB |
35.1KiB |
Bundle analysis report Branch upupming:feat/simple-bsi Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#5198 Bundle Size — 238.41KiB (-0.21%).79713c3(current) vs f24eb4a main#5196(baseline) Warning Bundle introduced one new package: @upupming/preact – View changed packages Bundle metrics
|
| Current #5198 |
Baseline #5196 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
 Cache Invalidation |
38.99% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
159(-2.45%) |
163 |
 Duplicate Modules |
64(-4.48%) |
67 |
| Duplicate Code |
45.81%(-2.09%) |
46.79% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 improvement
| Current #5198 |
Baseline #5196 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
92.65KiB (-0.53%) |
93.14KiB |
Bundle analysis report Branch upupming:feat/simple-bsi Project dashboard
Generated by RelativeCI Documentation Report issue
Merging this PR will degrade performance by 27.52%
Performance Changes
Comparing 
Footnotes
|
upupming
force-pushed
the
feat/simple-bsi
branch
from
b2e52c7 to
60f25fe
Compare
upupming
force-pushed
the
feat/simple-bsi
branch
from
60f25fe to
462a05e
Compare
upupming
force-pushed
the
feat/simple-bsi
branch
from
d38e449 to
d9c3120
Compare
This reverts commit d9c3120.
upupming
force-pushed
the
feat/simple-bsi
branch
from
c61e8c9 to
04eb7c9
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react/package.json (1)
117-119: Remove stale typesVersions path for deleted export
typesVersions["internal/document"]remains while the runtime export was removed. This misleads TS users into a type-only path that fails at runtime. Drop the mapping.- "internal/document": [ - "./runtime/lib/document.d.ts" - ],
🧹 Nitpick comments (22)
packages/react/runtime/__test__/utils/debug.js (1)
12-12: Defensive: tolerate non-instance children in testsGuard
toJSON()to avoid brittle failures if a primitive sneaks intochildNodes.- children: this.childNodes.map(child => child.toJSON()), + children: this.childNodes.map(child => + typeof child?.toJSON === 'function' ? child.toJSON() : child + ),Also applies to: 29-29
packages/react/runtime/__test__/lifecycle/isRendering.test.jsx (1)
45-45: Reduce brittleness: centralize access to background text valueThe repeated
__root.firstChild.firstChild.__valuesis fragile. Use a tiny helper to keep tests stable if the structure shifts again.- expect(__root.firstChild.firstChild.__values).toEqual(['Hello World']); + expect(rootTextValues()).toEqual(['Hello World']); @@ - expect(__root.firstChild.firstChild.__values).toEqual(['World']); + expect(rootTextValues()).toEqual(['World']); @@ - expect(__root.firstChild.firstChild.__values).toEqual(['World']); + expect(rootTextValues()).toEqual(['World']);Add near imports:
const rootTextValues = () => __root.firstChild.firstChild.__values;Also applies to: 90-90, 112-112
packages/react/runtime/__test__/render.test.jsx (1)
45-45: Same helper here to avoid hard-coding internalsMirror the accessor helper to reduce churn across tests.
- expect(__root.firstChild.firstChild.__values).toEqual(['Hello World']); + expect(rootTextValues()).toEqual(['Hello World']); @@ - expect(__root.firstChild.firstChild.__values).toEqual([88]); + expect(rootTextValues()).toEqual([88]);Add near imports:
const rootTextValues = () => __root.firstChild.firstChild.__values;Also applies to: 65-65
packages/react/runtime/src/lynx.ts (1)
56-56: Optional: don’t clobber host-provided setup — use nullish assignmentReplace direct assignment so a host-provided setup isn't overwritten.
- options.setupDom = setupDom; + options.setupDom ??= setupDom;Also: assignments to
options.documentremain in tests — packages/react/testing-library/src/vitest-global-setup.js:133,141. Remove or justify those usages.packages/react/runtime/__test__/debug/printSnapshot.test.jsx (1)
10-10: Remove unused import
BackgroundSnapshotInstanceisn’t used after the migration.-import { BackgroundSnapshotInstance, setupDom } from '../../src/backgroundSnapshot'; +import { setupDom } from '../../src/backgroundSnapshot';packages/react/runtime/__test__/debug/profile.test.jsx (1)
21-24: Add symmetric cleanup to avoid cross-test leakage.Optional but safer to unmount after each test.
Add:
-import { beforeAll, beforeEach, describe, expect, test, vi } from 'vitest'; +import { beforeAll, beforeEach, afterEach, describe, expect, test, vi } from 'vitest'; @@ beforeEach(() => { snapshotInstanceManager.clear(); scratch = options.setupDom({ type: 'root' }); }); + + afterEach(() => { + render(null, scratch); + });packages/react/runtime/src/snapshot/spread.ts (1)
92-101: Nit: preferObject.keys/entriesto avoid inherited keys.Current
for...inis safe for plain objects but usingObject.entriesis clearer and avoids surprises.- for (const key in newValue) { - const v = newValue[key]; + for (const [key, v] of Object.entries(newValue)) { @@ - for (const key in oldValue) { + for (const key of Object.keys(oldValue)) {Also applies to: 192-201
packages/react/runtime/src/snapshot.ts (2)
9-15: Update docstring to match the new background type.Comment still references
BackgroundSnapshotInstance. Recommend renaming toBackgroundDOM.- * 3. {@link BackgroundSnapshotInstance}: Runtime instance in the background thread + * 3. {@link BackgroundDOM}: Runtime instance in the background thread
170-193: Minor readability: avoid parsing index when addressing__extraProps.Not an issue, but you can branch on
res[1] === '__extraProps'before computingexpIndex.- const expIndex = Number(res[1]); - const ctx = this.values.get(id); + const isExtra = res[1] === '__extraProps'; + const expIndex = isExtra ? -1 : Number(res[1]); + const ctx = this.values.get(id); @@ - if (res[1] === '__extraProps') { + if (isExtra) {packages/react/runtime/src/lynx/tt.ts (1)
118-123: Nit: fix stale comment type.The nearby commented debug line still mentions
BackgroundSnapshotInstance.- // printSnapshotInstance(__root as BackgroundSnapshotInstance); + // printSnapshotInstance(__root as BackgroundDOM);packages/react/runtime/__test__/compat/initData.test.jsx (1)
36-36: Prefer direct setupDom import for clarity in testsIf you need decoration, keep the wrapper but use a local alias to reduce coupling to Preact.options.
- scratch = options.setupDom({ type: 'root' }); + const createDom = options.setupDom; + scratch = createDom({ type: 'root' });packages/react/runtime/src/internal.ts (1)
21-21: Prefer separate type re-export to avoid mixing with value exportsMinor ergonomics/readability.
-export { type BackgroundDOM, SnapshotInstance, snapshotCreateList, createSnapshot, snapshotManager }; +export type { BackgroundDOM } from './backgroundSnapshot.js'; +export { SnapshotInstance, snapshotCreateList, createSnapshot, snapshotManager };packages/react/runtime/__test__/element.test.jsx (2)
3-3: Use the public path consistentlyIf other tests import from runtime/lib, keep imports aligned to avoid duplicate module instances under different paths.
-import { setupDom } from '../src/backgroundSnapshot'; +import { setupDom } from '../src/backgroundSnapshot'; +# or align with other tests if they use '../../runtime/lib/backgroundSnapshot.js'
28-31: Assert sibling invariants tooAdd checks for nextSibling/previousSibling when removing to cover link maintenance.
root.removeChild(child2); - expect(child2.parentNode).toEqual(undefined); + expect(child2.parentNode).toEqual(undefined); + expect(child1.nextSibling).toBe(child3); + expect(child3.previousSibling).toBe(child1);Also applies to: 35-35
packages/react/runtime/src/root.ts (1)
36-36: Drop the double cast; make setupDom return BackgroundDOM.The
as unknown as BackgroundDOMis a smell and hides real typing issues. Prefer making setupDom’s return typeBackgroundDOMand using it directly.Apply this diff here once setupDom is typed correctly:
- setRoot(setupDom({ type: 'root' } as unknown as BackgroundDOM)); + setRoot(setupDom({ type: 'root' }));Outside this hunk, ensure setupDom is declared as:
export function setupDom(config: { type: string }): BackgroundDOM;packages/react/runtime/__test__/utils/envManager.ts (2)
43-49: Avoid proto; use Object.setPrototypeOf for clarity and perf.Functionally equivalent, less magical, and avoids TS suppression.
- // @ts-expect-error expected - vnode.__proto__ = SnapshotInstance.prototype; + Object.setPrototypeOf(vnode, SnapshotInstance.prototype);
61-63: Remove redundant cast; fix setupDom typing instead.Casting to
unknown as BackgroundDOMmasks real type info and can hide bugs.- setRoot(setupDom({ type: 'root' } as unknown as BackgroundDOM)); + setRoot(setupDom({ type: 'root' }));If needed, adjust setupDom’s signature to return BackgroundDOM.
packages/react/runtime/src/lynx/suspense.ts (1)
20-24: Minor: initialize ref with null for explicitness.Not required, but
useRef<BackgroundDOM | null>(null)makes intent clearer in strict setups.- const childrenRef = useRef<BackgroundDOM>(); + const childrenRef = useRef<BackgroundDOM | null>(null);And check
childrenRef.currentaccordingly.packages/react/runtime/__test__/hooks/useLynxGlobalEventListener.test.jsx (1)
20-23: Fix JSDoc type for background rootscratch is a Background DOM vnode, not a SnapshotInstance. Update the type to improve editor/type tooling.
- /** @type {SnapshotInstance} */ + /** @type {import('../../src/backgroundSnapshot').BackgroundDOM} */ let scratch; let switchToBackground = globalEnvManager.switchToBackground.bind(globalEnvManager);packages/react/runtime/__test__/preact.test.jsx (3)
5-5: Remove unused importsetupDom isn’t used in this file; drop it to avoid lint noise.
-import { hydrate, setupDom } from '../src/backgroundSnapshot'; +import { hydrate } from '../src/backgroundSnapshot';
316-318: Correct background scratch type annotationThe background scratch is a BackgroundDOM vnode, not a SnapshotInstance.
- /** @type {SnapshotInstance} */ + /** @type {import('../src/backgroundSnapshot').BackgroundDOM} */ let scratch; @@ - scratch = options.setupDom({ type: 'root' }); + scratch = options.setupDom({ type: 'root' });Also applies to: 340-342
624-639: DRY the setupDom-toJSON wrappingThis block duplicates the earlier wrapping logic. Consider extracting a small helper (e.g., withBackgroundToJSON) to apply the wrapper once per suite and restore it, reducing maintenance.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
.changeset/slick-pandas-flow.md(1 hunks)benchmark/react/cases/004-various-update/index.tsx(2 hunks)packages/react/package.json(1 hunks)packages/react/runtime/__test__/compat.test.jsx(2 hunks)packages/react/runtime/__test__/compat/initData.test.jsx(2 hunks)packages/react/runtime/__test__/debug/printSnapshot.test.jsx(3 hunks)packages/react/runtime/__test__/debug/profile.test.jsx(1 hunks)packages/react/runtime/__test__/element.test.jsx(2 hunks)packages/react/runtime/__test__/hooks/useLynxGlobalEventListener.test.jsx(2 hunks)packages/react/runtime/__test__/hydrate.test.jsx(7 hunks)packages/react/runtime/__test__/lifecycle/isRendering.test.jsx(3 hunks)packages/react/runtime/__test__/preact.test.jsx(17 hunks)packages/react/runtime/__test__/render.test.jsx(2 hunks)packages/react/runtime/__test__/renderToOpcodes.test.jsx(1 hunks)packages/react/runtime/__test__/snapshot/spread.test.jsx(2 hunks)packages/react/runtime/__test__/snapshotPatch.test.jsx(15 hunks)packages/react/runtime/__test__/utils/debug.js(2 hunks)packages/react/runtime/__test__/utils/envManager.ts(3 hunks)packages/react/runtime/__test__/utils/setup.js(0 hunks)packages/react/runtime/lazy/internal.js(0 hunks)packages/react/runtime/src/backgroundSnapshot.ts(4 hunks)packages/react/runtime/src/debug/printSnapshot.ts(1 hunks)packages/react/runtime/src/document.ts(0 hunks)packages/react/runtime/src/internal.ts(2 hunks)packages/react/runtime/src/lifecycle/patch/commit.ts(3 hunks)packages/react/runtime/src/lynx.ts(2 hunks)packages/react/runtime/src/lynx/suspense.ts(2 hunks)packages/react/runtime/src/lynx/tt.ts(2 hunks)packages/react/runtime/src/root.ts(2 hunks)packages/react/runtime/src/snapshot.ts(2 hunks)packages/react/runtime/src/snapshot/spread.ts(2 hunks)packages/react/runtime/types/internal-preact.d.ts(2 hunks)packages/react/testing-library/src/vitest-global-setup.js(2 hunks)
💤 Files with no reviewable changes (3)
- packages/react/runtime/lazy/internal.js
- packages/react/runtime/test/utils/setup.js
- packages/react/runtime/src/document.ts
🧰 Additional context used
🧠 Learnings (11)
📚 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/react/runtime/src/internal.tspackages/react/runtime/src/lynx.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/react/runtime/src/internal.tspackages/react/runtime/src/lynx.tspackages/react/runtime/src/root.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/slick-pandas-flow.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/slick-pandas-flow.mdpackages/react/runtime/src/lynx.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/slick-pandas-flow.md
📚 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/slick-pandas-flow.md
📚 Learning: 2025-08-19T11:25:36.127Z
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).
Applied to files:
.changeset/slick-pandas-flow.md
📚 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/__test__/debug/profile.test.jsxpackages/react/runtime/__test__/compat/initData.test.jsx
📚 Learning: 2025-08-11T05:57:18.212Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1305
File: packages/testing-library/testing-environment/src/index.ts:255-258
Timestamp: 2025-08-11T05:57:18.212Z
Learning: In the ReactLynx testing environment (`packages/testing-library/testing-environment/src/index.ts`), the dual assignment pattern `target.console.method = console.method = () => {}` is required for rstest compatibility. This is because rstest provides `console` in an IIFE (Immediately Invoked Function Expression), and both the target and global console need to have these methods defined for proper test execution.
Applied to files:
packages/react/runtime/__test__/compat.test.jsxpackages/react/runtime/__test__/hooks/useLynxGlobalEventListener.test.jsx
📚 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__/compat.test.jsx
📚 Learning: 2025-08-11T05:59:28.530Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1305
File: packages/react/testing-library/src/plugins/vitest.ts:4-6
Timestamp: 2025-08-11T05:59:28.530Z
Learning: In the lynx-family/lynx-stack repository, the `packages/react/testing-library` package does not have `vite` as a direct dependency. It relies on `vitest` being available from the monorepo root and accesses Vite types through re-exports from `vitest/node`. Direct imports from `vite` should not be suggested for this package.
Applied to files:
packages/react/runtime/__test__/compat.test.jsx
⏰ 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). (16)
- GitHub Check: Playwright MULTI_THREAD-CSR (2/4) / check
- GitHub Check: Playwright MULTI_THREAD-CSR (1/4) / check
- GitHub Check: Playwright ALL_ON_UI-SSR (1/4) / check
- GitHub Check: Playwright ALL_ON_UI-CSR (3/4) / check
- GitHub Check: Playwright ALL_ON_UI-CSR (2/4) / check
- GitHub Check: Playwright ALL_ON_UI-CSR (1/4) / check
- GitHub Check: Playwright ALL_ON_UI-SSR (2/4) / check
- GitHub Check: Playwright ALL_ON_UI-SSR (3/4) / check
- GitHub Check: Playwright MULTI_THREAD-CSR (3/4) / check
- GitHub Check: Playwright ALL_ON_UI-CSR (4/4) / check
- GitHub Check: Vitest (Ubuntu) / check
- GitHub Check: Vitest (Windows) / check
- GitHub Check: test-publish / check
- GitHub Check: lighthouse / check
- GitHub Check: eslint / check
- GitHub Check: benchmark / nodejs-benchmark
🔇 Additional comments (32)
packages/react/package.json (1)
173-173: Confirm forked preact pin, update deprecated usages, and check for conflicts
- Pin published: npm shows @upupming/preact@10.24.0-simple-bsi.4.
- Types include setupDom: packages/react/runtime/types/internal-preact.d.ts — Options.setupDom?: typeof setupDom.
- Deprecated usage found: packages/react/testing-library/src/vitest-global-setup.js assigns options.document (≈lines 133, 141) — replace with setupDom or verify compatibility.
- Verify no other package pins a conflicting preact alias/version across the monorepo (search package.json files for "preact" / "npm:@upupming/preact").
packages/react/runtime/src/lynx.ts (1)
9-9: LGTM: new DOM setup is correctly wiredImporting
setupDomhere aligns with the background migration.packages/react/runtime/__test__/renderToOpcodes.test.jsx (1)
606-606: LGTM: using SnapshotInstance directly simplifies setupDirect instantiation is clearer than the prior document shim and matches new APIs.
packages/react/runtime/__test__/debug/printSnapshot.test.jsx (1)
54-57: LGTM: migrated construction to setupDomThe switch to
setupDom({ type })matches the new Background DOM contract and keeps the test behavior intact.Also applies to: 76-79
packages/react/runtime/__test__/debug/profile.test.jsx (1)
8-8: No action — options.setupDom is initialized in the Vitest setup.packages/react/runtime/vitest.config.ts registers setupFiles: test/utils/globals.js then test/utils/setup.js; globals.js sets BACKGROUND = true and setup.js imports packages/react/runtime/src/lynx.ts, which assigns options.setupDom = setupDom when BACKGROUND is true.
packages/react/runtime/__test__/compat.test.jsx (1)
23-29: LGTM: switching test root tonew SnapshotInstance('root').This aligns with the new DOM model and keeps cleanup deterministic via
render(null, scratch).packages/react/runtime/__test__/snapshotPatch.test.jsx (2)
18-19: LGTM: usingsetupDomin place ofBackgroundSnapshotInstance.Import and usage match the new API.
81-83: Counters are already reset in the test setup — the “reset counters” concern is incorrect; optional: make snapshots ID‑agnostic.Global setup (packages/react/testing-library/src/vitest-global-setup.js) calls snapshotInstanceManager.clear()/backgroundSnapshotInstanceManager.clear() and sets nextId = 0; runtime/test/utils/envManager.ts.resetEnv() does the same; many tests also reset these in beforeEach (e.g. spread.test.jsx, hydrate.test.jsx).
- Optional: prefer pattern assertions for __id fields or add a small helper (e.g., const newDom = (type) => setupDom({ type })) to reduce snapshot brittleness.
Likely an incorrect or invalid review comment.
packages/react/runtime/src/snapshot/spread.ts (1)
14-15: Type shift toBackgroundDOMis correct and non-breaking.
transformSpreadacceptingBackgroundDOM | SnapshotInstancemakes shared logic usable on both sides.Also applies to: 298-301
packages/react/runtime/src/debug/printSnapshot.ts (1)
8-14: LGTM:BackgroundDOMunion typing.Printing works for both background DOM and main-thread instances without behavior change.
packages/react/runtime/src/snapshot.ts (1)
139-147: LGTM: background manager now tracksBackgroundDOM.Type change is consistent with the refactor; methods (
updateId,getValueBySign) remain compatible.packages/react/runtime/src/lynx/tt.ts (1)
90-95: LGTM: hydrate now takesBackgroundDOM.Matches the new
hydrate(before, __root as BackgroundDOM)signature.packages/react/runtime/src/lifecycle/patch/commit.ts (1)
139-141: Guarded teardown looks goodUsing the map to resolve the root node and deferring teardown is consistent with prior flow.
packages/react/runtime/src/internal.ts (1)
10-10: Type-only import is correct — BackgroundDOM is exported as an interface.
packages/react/runtime/src/backgroundSnapshot.ts exportsexport interface BackgroundDOM, soimport type { BackgroundDOM } from './backgroundSnapshot.js';is valid.packages/react/runtime/__test__/element.test.jsx (1)
9-12: Good migration to setupDomRoot/children creation via setupDom matches the new BackgroundDOM model.
packages/react/testing-library/src/vitest-global-setup.js (1)
18-18: Great: background root now uses setupDomThis aligns tests with the new DOM surface.
packages/react/runtime/__test__/hydrate.test.jsx (4)
3-3: Import swap to setupDom/hydrate looks correct.Path and named imports align with the new API.
239-243: Root bootstrap via setupDom.Good coverage for the empty-render path using a BackgroundDOM root.
295-303: Slot case migration to setupDom.Looks consistent; keeps multi-children/slot behavior intact.
93-111: Parity confirmed — setupDom exposes SnapshotInstance API.backgroundSnapshot.setupDom (packages/react/runtime/src/backgroundSnapshot.ts) attaches insertBefore/setAttribute to the vnode and the test env (packages/react/runtime/test/utils/envManager.ts) maps vnodes to SnapshotInstance.prototype; no change required.
packages/react/runtime/__test__/snapshot/spread.test.jsx (4)
6-6: Importing preact options/render is fine with the new hook.No issues spotted.
9-10: Adoption of hydrate/setupDom.Migration matches the new background DOM API.
13-18: Expanded snapshot imports are consistent with env changes.OK to use SnapshotInstance alongside setupDom within the same test.
31-35: envManager already wires options.setupDom — no change required.options.setupDom is assigned in packages/react/runtime/test/utils/envManager.ts (lines 43 and 68); adding a sanity assertion is optional.
packages/react/runtime/src/root.ts (2)
4-6: Type import + setupDom import align with new BackgroundDOM.Looks good.
12-12: __root union updated to include BackgroundDOM.This keeps types accurate across both environments.
packages/react/runtime/__test__/utils/envManager.ts (2)
13-15: Using preact options.setupDom is the right direction.Imports are consistent with the new architecture.
68-69: Restore original options.setupDom after testsPersisting a mutated global hook across suites can cause flaky tests. File: packages/react/runtime/test/utils/envManager.ts (around lines 68–69).
Suggested minimal change:
// add to class fields private prevSetupDom?: typeof options.setupDom; // when assigning: this.prevSetupDom = options.setupDom; options.setupDom = setupDom; // in resetEnv(): if (this.prevSetupDom !== undefined) { options.setupDom = this.prevSetupDom; this.prevSetupDom = undefined; }packages/react/runtime/src/lynx/suspense.ts (2)
11-12: Type shift to BackgroundDOM is correct.Keeps the wrapper refs aligned with the new background DOM abstraction.
32-33: Ref cleanup logic is intact after type change.Accessing
__idremains valid with BackgroundDOM.packages/react/runtime/src/backgroundSnapshot.ts (2)
320-341: No change required — traverseSnapshotInstance accepts BackgroundDOMBackgroundDOM declares childNodes: BackgroundDOM[] and traverseSnapshotInstance(si, ...) is structurally compatible, so removeChild’s traversal is type-safe (see snapshot.ts traverseSnapshotInstance and backgroundSnapshot.ts BackgroundDOM).
234-290: Cross-parent moves are already handled — no RemoveChild needed.Verified: backgroundSnapshot.insertBefore emits SnapshotOperation.InsertBefore for moves (reconstructInstanceTree used only when node.__removed_from_tree). The main-thread consumers apply InsertBefore as a move: OffscreenElement.insertBefore calls node._remove() then emits OperationType.InsertBefore, and initOffscreenDocument.decodeOperation handles InsertBefore by calling target.insertBefore(kid, ref) (which moves the node). No code path requires emitting RemoveChild before InsertBefore.
Files checked: packages/react/runtime/src/backgroundSnapshot.ts, packages/web-platform/offscreen-document/src/webworker/OffscreenElement.ts, packages/web-platform/offscreen-document/src/main/initOffscreenDocument.ts.
| "@lynx-js/react": patch | ||
| --- | ||
|
|
||
| Reuse vnode tree as dom tree in bts |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify scope and bump version; this is a breaking surface change
Removing the public ./internal/document export and switching from options.document to options.setupDom is breaking for external consumers. Promote to a major (or at least minor if you consider internal/* non-semver) and spell out the migration in the note.
Apply:
-"@lynx-js/react": patch
+"@lynx-js/react": major
@@
-Reuse vnode tree as dom tree in bts
+feat(background): reuse VNode tree as Background DOM via options.setupDom
+BREAKING: remove options.document and the public export "./internal/document". Migrate any usage to options.setupDom and BackgroundDOM.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "@lynx-js/react": patch | |
| --- | |
| Reuse vnode tree as dom tree in bts | |
| "@lynx-js/react": major | |
| --- | |
| feat(background): reuse VNode tree as Background DOM via options.setupDom | |
| BREAKING: remove options.document and the public export "./internal/document". Migrate any usage to options.setupDom and BackgroundDOM. |
🤖 Prompt for AI Agents
In .changeset/slick-pandas-flow.md around lines 2–5, the changeset currently
lists this as a patch and omits migration guidance despite removing the public
./internal/document export and replacing options.document with options.setupDom;
update the changeset to a major (or at least minor) release, explicitly mark it
as a breaking change, and add a short migration note explaining: 1) consumers
must switch from options.document to options.setupDom and show the minimal
example of the new API call, 2) any code importing ./internal/document must be
replaced with the new public API (or migrated to internal usage if appropriate),
and 3) recommend steps to test the change; also update the summary line to
clearly state the scope of the breaking change.
There was a problem hiding this comment.
@coderabbitai minor please, we are in 0.x stage
There was a problem hiding this comment.
@upupming You're absolutely right! In 0.x stage, breaking changes should go in minor releases. Here's the updated changeset:
---
"@lynx-js/react": minor
---
feat(background): reuse VNode tree as Background DOM via options.setupDom
BREAKING: remove options.document and the public export "./internal/document". Migrate any usage to options.setupDom and BackgroundDOM.This properly reflects the 0.x semver convention where breaking changes are released as minor bumps rather than major ones.
🧠 Learnings used
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.
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.
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.
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#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.
| options, | ||
| 'setupDom', | ||
| function(old: typeof options.setupDom, vnode: BackgroundDOM) { | ||
| old?.(vnode); | ||
| const oldSetAttribute = vnode.setAttribute; | ||
| vnode.setAttribute = function(this: BackgroundDOM, key, value) { | ||
| const values = value as unknown[]; | ||
| if ( | ||
| key === 'values' | ||
| && values[values.length - 1] === 'stop-benchmark-true' | ||
| ) { | ||
| // we only care about the update that stops the benchmark | ||
|
|
||
| Codspeed.startBenchmark(); | ||
| const ret = oldSetAttribute.call(this, key, value); | ||
| Codspeed.stopBenchmark(); | ||
| Codspeed.setExecutedBenchmark( | ||
| `${__REPO_FILEPATH__}::${__webpack_chunkname__}-setAttribute__BatchedValues`, | ||
| ); | ||
| return ret; | ||
| } | ||
|
|
||
| return old!.call(this, key, value); | ||
| }, | ||
| return oldSetAttribute.call(this, key, value); | ||
| }; | ||
| return vnode; | ||
| } as never, | ||
| ); |
There was a problem hiding this comment.
Preserve setupDom return value and type the setAttribute shim correctly
- old?.(vnode) might return a (possibly replaced) vnode; reassign it.
- Align setAttribute signature with BackgroundDOM (string | number, unknown).
hook(
options,
'setupDom',
function(old: typeof options.setupDom, vnode: BackgroundDOM) {
- old?.(vnode);
- const oldSetAttribute = vnode.setAttribute;
- vnode.setAttribute = function(this: BackgroundDOM, key, value) {
+ vnode = (old?.(vnode) as BackgroundDOM | undefined) ?? vnode;
+ const oldSetAttribute = vnode.setAttribute;
+ vnode.setAttribute = function(this: BackgroundDOM, key: string | number, value: unknown) {
const values = value as unknown[];
- if (
- key === 'values'
+ if (
+ key === 'values'
&& values[values.length - 1] === 'stop-benchmark-true'
) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| options, | |
| 'setupDom', | |
| function(old: typeof options.setupDom, vnode: BackgroundDOM) { | |
| old?.(vnode); | |
| const oldSetAttribute = vnode.setAttribute; | |
| vnode.setAttribute = function(this: BackgroundDOM, key, value) { | |
| const values = value as unknown[]; | |
| if ( | |
| key === 'values' | |
| && values[values.length - 1] === 'stop-benchmark-true' | |
| ) { | |
| // we only care about the update that stops the benchmark | |
| Codspeed.startBenchmark(); | |
| const ret = oldSetAttribute.call(this, key, value); | |
| Codspeed.stopBenchmark(); | |
| Codspeed.setExecutedBenchmark( | |
| `${__REPO_FILEPATH__}::${__webpack_chunkname__}-setAttribute__BatchedValues`, | |
| ); | |
| return ret; | |
| } | |
| return old!.call(this, key, value); | |
| }, | |
| return oldSetAttribute.call(this, key, value); | |
| }; | |
| return vnode; | |
| } as never, | |
| ); | |
| options, | |
| 'setupDom', | |
| function(old: typeof options.setupDom, vnode: BackgroundDOM) { | |
| vnode = (old?.(vnode) as BackgroundDOM | undefined) ?? vnode; | |
| const oldSetAttribute = vnode.setAttribute; | |
| vnode.setAttribute = function(this: BackgroundDOM, key: string | number, value: unknown) { | |
| const values = value as unknown[]; | |
| if ( | |
| key === 'values' | |
| && values[values.length - 1] === 'stop-benchmark-true' | |
| ) { | |
| // we only care about the update that stops the benchmark | |
| Codspeed.startBenchmark(); | |
| const ret = oldSetAttribute.call(this, key, value); | |
| Codspeed.stopBenchmark(); | |
| Codspeed.setExecutedBenchmark( | |
| `${__REPO_FILEPATH__}::${__webpack_chunkname__}-setAttribute__BatchedValues`, | |
| ); | |
| return ret; | |
| } | |
| return oldSetAttribute.call(this, key, value); | |
| }; | |
| return vnode; | |
| } as never, | |
| ); |
🤖 Prompt for AI Agents
In benchmark/react/cases/004-various-update/index.tsx around lines 91-117, the
shim calls old?.(vnode) but ignores its return (which may be a replaced vnode)
and the setAttribute shim signature is not aligned with BackgroundDOM; update
the code to reassign the vnode from old (e.g. vnode = old?.(vnode) as
BackgroundDOM || vnode) so you operate on and return the possibly replaced
vnode, and change the setAttribute signature to match BackgroundDOM (this:
BackgroundDOM, key: string | number, value: unknown) while preserving and
calling the original oldSetAttribute and returning its result.
| globalEnvManager.switchToBackground = () => { | ||
| switchToBackground(); | ||
| const oldSetupDom = options.setupDom; | ||
| options.setupDom = (vnode) => { | ||
| vnode = oldSetupDom(vnode); | ||
| vnode.toJSON = backgroundSnapshotInstanceToJSON; | ||
| return vnode; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Avoid global leakage and double-wrapping options.setupDom
Wrap once and restore in afterAll; otherwise tests may stack wrappers and leak into other files.
Apply:
- globalEnvManager.switchToBackground = () => {
- switchToBackground();
- const oldSetupDom = options.setupDom;
- options.setupDom = (vnode) => {
- vnode = oldSetupDom(vnode);
- vnode.toJSON = backgroundSnapshotInstanceToJSON;
- return vnode;
- };
- };
+ /** keep original to restore in afterAll */
+ /** @type {typeof options.setupDom | undefined} */
+ let originalSetupDom;
+ const DECORATED = Symbol.for('lynx.test.setupDomDecorated');
+ globalEnvManager.switchToBackground = () => {
+ switchToBackground();
+ if ((options.setupDom)[DECORATED]) return;
+ originalSetupDom = options.setupDom;
+ const base = originalSetupDom;
+ const wrapped = (vnode) => {
+ const dom = base(vnode);
+ dom.toJSON = backgroundSnapshotInstanceToJSON;
+ return dom;
+ };
+ wrapped[DECORATED] = true;
+ options.setupDom = wrapped;
+ };Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/react/runtime/__test__/compat/initData.test.jsx around lines 18-26,
the test overrides globalEnvManager.switchToBackground and replaces
options.setupDom on every call, causing double-wrapping and global leakage;
change this so you capture and store the original switchToBackground and
original options.setupDom once, perform the wrapper only once (not on every
call), and add an afterAll that restores both
globalEnvManager.switchToBackground and options.setupDom to their originals;
ensure the new switchToBackground simply delegates to the original switch
function without re-wrapping options.setupDom.
| afterAll(() => { | ||
| delete BackgroundSnapshotInstance.prototype.toJSON; | ||
| globalEnvManager.switchToBackground = switchToBackground; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also restore options.setupDom
Restore the original setupDom to avoid global state bleed.
Apply:
afterAll(() => {
globalEnvManager.switchToBackground = switchToBackground;
+ if (typeof originalSetupDom === 'function') options.setupDom = originalSetupDom;
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| globalEnvManager.switchToBackground = switchToBackground; | |
| afterAll(() => { | |
| globalEnvManager.switchToBackground = switchToBackground; | |
| if (typeof originalSetupDom === 'function') options.setupDom = originalSetupDom; | |
| }); |
🤖 Prompt for AI Agents
In packages/react/runtime/__test__/compat/initData.test.jsx around line 32, the
test replaces globalEnvManager.setupDom but doesn't restore it, causing global
state bleed; capture the original setupDom before assigning switchToBackground,
and after the test (or in an afterEach/finally) reassign
globalEnvManager.setupDom back to the saved original value so the global
environment is restored after the test.
| let switchToBackground = globalEnvManager.switchToBackground.bind(globalEnvManager); | ||
| const ee = new EventEmitter(); | ||
|
|
||
| beforeAll(() => { | ||
| setupBackgroundDocument(); | ||
| setupPage(__CreatePage('0', 0)); | ||
|
|
||
| BackgroundSnapshotInstance.prototype.toJSON = backgroundSnapshotInstanceToJSON; | ||
| globalEnvManager.switchToBackground = () => { | ||
| switchToBackground(); | ||
| const oldSetupDom = options.setupDom; | ||
| options.setupDom = (vnode) => { | ||
| vnode = oldSetupDom(vnode); | ||
| vnode.toJSON = backgroundSnapshotInstanceToJSON; | ||
| return vnode; | ||
| }; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid accumulating wrappers on options.setupDom; restore it after tests
Each call to switchToBackground wraps options.setupDom again, and afterAll only restores the switchToBackground function, not options.setupDom. Capture the original setupDom once and restore it; also wrap against that original to prevent wrapper stacking across tests.
- let switchToBackground = globalEnvManager.switchToBackground.bind(globalEnvManager);
+ let switchToBackground = globalEnvManager.switchToBackground.bind(globalEnvManager);
+ let originalSetupDom = options.setupDom;
beforeAll(() => {
setupPage(__CreatePage('0', 0));
globalEnvManager.switchToBackground = () => {
switchToBackground();
- const oldSetupDom = options.setupDom;
- options.setupDom = (vnode) => {
- vnode = oldSetupDom(vnode);
+ options.setupDom = (vnode) => {
+ vnode = originalSetupDom(vnode);
vnode.toJSON = backgroundSnapshotInstanceToJSON;
return vnode;
};
};
});
@@
afterAll(() => {
- globalEnvManager.switchToBackground = switchToBackground;
+ globalEnvManager.switchToBackground = switchToBackground;
+ options.setupDom = originalSetupDom;
});Also applies to: 49-51
🤖 Prompt for AI Agents
In packages/react/runtime/__test__/hooks/useLynxGlobalEventListener.test.jsx
around lines 22 to 37 (and similarly at 49-51), the test replaces
globalEnvManager.switchToBackground with a wrapper that repeatedly rewraps
options.setupDom on every call causing wrapper accumulation; capture the
original options.setupDom once (e.g., const originalSetupDom = options.setupDom)
before modifying switchToBackground, have the replacement wrap originalSetupDom
(not the current options.setupDom) and assign the wrapped function to
options.setupDom, and ensure afterAll restores options.setupDom =
originalSetupDom as well as restoring switchToBackground so successive tests
don’t stack wrappers.
| if (__globalSnapshotPatch) { | ||
| const oldValues = this.__values; | ||
| if (oldValues) { | ||
| for (let index = 0; index < (value as unknown[]).length; index++) { | ||
| const { needUpdate, valueToCommit } = setAttributeImpl( | ||
| this, | ||
| (value as unknown[])[index], | ||
| oldValues[index], | ||
| index, | ||
| ); | ||
| if (needUpdate) { | ||
| __globalSnapshotPatch.push( | ||
| SnapshotOperation.SetAttribute, | ||
| this.__id, | ||
| index, | ||
| valueToCommit, | ||
| ); | ||
| } | ||
| } | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle removed trailing values when diffing the values array
When new values is shorter than the old, removed indices aren’t patched. Diff up to max length to send removals (undefined) as needed.
- if (key === 'values') {
+ if (key === 'values') {
if (__globalSnapshotPatch) {
- const oldValues = this.__values;
- if (oldValues) {
- for (let index = 0; index < (value as unknown[]).length; index++) {
+ const oldValues = this.__values ?? [];
+ const newValues = value as unknown[]; // eslint-disable-line @typescript-eslint/no-unsafe-assignment
+ if (oldValues.length) {
+ const max = Math.max(oldValues.length, newValues.length);
+ for (let index = 0; index < max; index++) {
const { needUpdate, valueToCommit } = setAttributeImpl(
this,
- (value as unknown[])[index],
- oldValues[index],
+ newValues[index],
+ oldValues[index],
index,
);
if (needUpdate) {
__globalSnapshotPatch.push(
SnapshotOperation.SetAttribute,
this.__id,
index,
valueToCommit,
);
}
}
} else {
const patch = [];
- const length = (value as unknown[]).length;
- for (let index = 0; index < length; ++index) {
- const { valueToCommit } = setAttributeImpl(this, (value as unknown[])[index], null, index);
+ const length = newValues.length;
+ for (let index = 0; index < length; ++index) {
+ const { valueToCommit } = setAttributeImpl(this, newValues[index], null, index);
patch[index] = valueToCommit;
}
__globalSnapshotPatch.push(
SnapshotOperation.SetAttributes,
this.__id,
patch,
);
}
} else {
@@
- this.__values = value as unknown[];
+ this.__values = value as unknown[];Also applies to: 394-399, 408-417
🤖 Prompt for AI Agents
packages/react/runtime/src/backgroundSnapshot.ts around lines 350-369: when
diffing arrays the loop only iterates new values length so removed trailing old
items aren’t emitted; change the iteration to run to Math.max(oldValues.length,
(value as unknown[]).length) and for indexes where the new value is missing
treat it as undefined (call setAttributeImpl or push
SnapshotOperation.SetAttribute with undefined) so removals are patched; apply
the same max-length iteration and undefined-removal logic to the other affected
ranges (around 394-399 and 408-417).
| export function setupDom(vnode: BackgroundDOM): BackgroundDOM { | ||
| const type = vnode.type; | ||
| vnode.__snapshot_def = snapshotManager.values.get(type)!; | ||
| const id = vnode.__id = backgroundSnapshotInstanceManager.nextId += 1; | ||
| __globalSnapshotPatch?.push(SnapshotOperation.CreateElement, type, id); | ||
|
|
||
| vnode.contains = contains; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against unknown vnode types in setupDom
Throw a descriptive error if the type is not registered; the non-null assertion can mask integration issues.
export function setupDom(vnode: BackgroundDOM): BackgroundDOM {
const type = vnode.type;
- vnode.__snapshot_def = snapshotManager.values.get(type)!;
+ const def = snapshotManager.values.get(type);
+ if (!def) {
+ throw new Error(`setupDom: unknown vnode type "${type}"`);
+ }
+ vnode.__snapshot_def = def;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function setupDom(vnode: BackgroundDOM): BackgroundDOM { | |
| const type = vnode.type; | |
| vnode.__snapshot_def = snapshotManager.values.get(type)!; | |
| const id = vnode.__id = backgroundSnapshotInstanceManager.nextId += 1; | |
| __globalSnapshotPatch?.push(SnapshotOperation.CreateElement, type, id); | |
| vnode.contains = contains; | |
| export function setupDom(vnode: BackgroundDOM): BackgroundDOM { | |
| const type = vnode.type; | |
| const def = snapshotManager.values.get(type); | |
| if (!def) { | |
| throw new Error(`setupDom: unknown vnode type "${type}"`); | |
| } | |
| vnode.__snapshot_def = def; | |
| const id = vnode.__id = backgroundSnapshotInstanceManager.nextId += 1; | |
| __globalSnapshotPatch?.push(SnapshotOperation.CreateElement, type, id); | |
| vnode.contains = contains; | |
| } |
🤖 Prompt for AI Agents
packages/react/runtime/src/backgroundSnapshot.ts around lines 510 to 516:
setupDom currently uses a non-null assertion when reading
snapshotManager.values.get(type), which can hide integration bugs; replace the
direct get+non-null with a guarded lookup: retrieve the snapshotDef via
get(type), if undefined throw a descriptive Error that includes the unknown type
(and optionally list registered types), otherwise assign vnode.__snapshot_def to
the retrieved value and proceed; ensure the thrown message makes it clear the
vnode type was unregistered so callers can trace the registration issue.
| export function tearDown(dom: BackgroundDOM): void { | ||
| traverseSnapshotInstance(dom, v => { | ||
| v.parentNode = undefined; | ||
| v.previousSibling = undefined; | ||
| v.nextSibling = undefined; | ||
| backgroundSnapshotInstanceManager.values.delete(v.__id); | ||
| }); | ||
| } |
There was a problem hiding this comment.
tearDown mutates links during traversal; risk of skipping nodes
If traverseSnapshotInstance walks via nextSibling/child links, clearing them inside the callback can cut traversal short and leak nodes in the manager.
Apply a two-pass approach:
-export function tearDown(dom: BackgroundDOM): void {
- traverseSnapshotInstance(dom, v => {
- v.parentNode = undefined;
- v.previousSibling = undefined;
- v.nextSibling = undefined;
- backgroundSnapshotInstanceManager.values.delete(v.__id);
- });
-}
+export function tearDown(dom: BackgroundDOM): void {
+ const nodes: BackgroundDOM[] = [];
+ traverseSnapshotInstance(dom, v => { nodes.push(v); });
+ // Drop from manager first
+ for (const v of nodes) backgroundSnapshotInstanceManager.values.delete(v.__id);
+ // Then sever links
+ for (const v of nodes) {
+ v.parentNode = undefined;
+ v.previousSibling = undefined;
+ v.nextSibling = undefined;
+ // Best-effort extra cleanup to reduce retention
+ if ('firstChild' in (v as any)) (v as any).firstChild = undefined;
+ if ('lastChild' in (v as any)) (v as any).lastChild = undefined;
+ if (Array.isArray((v as any).childNodes)) (v as any).childNodes.length = 0;
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function tearDown(dom: BackgroundDOM): void { | |
| traverseSnapshotInstance(dom, v => { | |
| v.parentNode = undefined; | |
| v.previousSibling = undefined; | |
| v.nextSibling = undefined; | |
| backgroundSnapshotInstanceManager.values.delete(v.__id); | |
| }); | |
| } | |
| export function tearDown(dom: BackgroundDOM): void { | |
| const nodes: BackgroundDOM[] = []; | |
| traverseSnapshotInstance(dom, v => { nodes.push(v); }); | |
| // Drop from manager first | |
| for (const v of nodes) backgroundSnapshotInstanceManager.values.delete(v.__id); | |
| // Then sever links | |
| for (const v of nodes) { | |
| v.parentNode = undefined; | |
| v.previousSibling = undefined; | |
| v.nextSibling = undefined; | |
| // Best-effort extra cleanup to reduce retention | |
| if ('firstChild' in (v as any)) (v as any).firstChild = undefined; | |
| if ('lastChild' in (v as any)) (v as any).lastChild = undefined; | |
| if (Array.isArray((v as any).childNodes)) (v as any).childNodes.length = 0; | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/react/runtime/src/lifecycle/patch/commit.ts around lines 95 to 102,
tearDown currently mutates nextSibling/child links while
traverseSnapshotInstance is walking those links which can cause traversal to
skip nodes and leak entries; change to a two-pass approach: first traverse and
collect all snapshot instances (or their ids) into a temporary array without
mutating, then iterate that array to clear parent/previousSibling/nextSibling
and delete each id from backgroundSnapshotInstanceManager.values so traversal is
not affected by in-place mutations.
| import type { BackgroundSnapshotInstance, setupDom } from '../src/backgroundSnapshot.ts'; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix type drift: replace BackgroundSnapshotInstance with BackgroundDOM.
The rest of the codebase moved to BackgroundDOM. Keeping the old type here will break consumers and mis-type options.vnode.
-import type { BackgroundSnapshotInstance, setupDom } from '../src/backgroundSnapshot.ts';
+import type { BackgroundDOM, setupDom } from '../src/backgroundSnapshot.js';
@@
- /** vnode */
- vnode?(vnode: VNode & BackgroundSnapshotInstance): void;
+ /** vnode */
+ vnode?(vnode: VNode & BackgroundDOM): void;
setupDom?: typeof setupDom;If your build expects extensionless imports in .d.ts, use '../src/backgroundSnapshot' instead of .js.
Check for any remaining references to the old type:
Also applies to: 25-27
🏁 Script executed:
#!/bin/bash
rg -n "BackgroundSnapshotInstance" packages/react/runtime -g '!**/dist/**'Length of output: 0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== rg: BackgroundSnapshotInstance ==="
rg -n --hidden -S "BackgroundSnapshotInstance" -g '!**/dist/**' || true
echo
echo "=== rg: BackgroundDOM ==="
rg -n --hidden -S "BackgroundDOM" -g '!**/dist/**' || true
echo
echo "=== fd: backgroundSnapshot.* files ==="
fd -HI "backgroundSnapshot.*" || true
echo
echo "=== show: packages/react/runtime/types/internal-preact.d.ts ==="
if [ -f "packages/react/runtime/types/internal-preact.d.ts" ]; then
sed -n '1,240p' packages/react/runtime/types/internal-preact.d.ts
else
echo "file not found: packages/react/runtime/types/internal-preact.d.ts"
fi
echo
echo "=== rg: backgroundSnapshot under packages/react/runtime ==="
rg -n --hidden -S "backgroundSnapshot" packages/react/runtime -g '!**/dist/**' || true
echo
echo "=== cat any backgroundSnapshot.* files found under packages/react/runtime (first 240 lines each) ==="
while IFS= read -r f; do
echo "----- $f -----"
sed -n '1,240p' "$f"
done < <(fd -HI "backgroundSnapshot.*" packages/react/runtime || true)Length of output: 1884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== file exists ==="
[ -f packages/react/runtime/src/backgroundSnapshot.ts ] && echo "yes" || echo "no"
echo
echo "=== file contents (first 400 lines) ==="
sed -n '1,400p' packages/react/runtime/src/backgroundSnapshot.ts || true
echo
echo "=== lines with 'Background' in that file ==="
rg -n "Background" packages/react/runtime/src/backgroundSnapshot.ts || true
echo
echo "=== repo search: BackgroundDOM ==="
rg -n --hidden -S "BackgroundDOM" -g '!**/dist/**' || true
echo
echo "=== repo search: BackgroundSnapshotInstance ==="
rg -n --hidden -S "BackgroundSnapshotInstance" -g '!**/dist/**' || true
echo
echo "=== grep fallback: BackgroundDOM ==="
grep -RIn "BackgroundDOM" . --exclude-dir=dist || true
echo
echo "=== grep fallback: BackgroundSnapshotInstance ==="
grep -RIn "BackgroundSnapshotInstance" . --exclude-dir=dist || trueLength of output: 23717
Replace BackgroundSnapshotInstance with BackgroundDOM in packages/react/runtime/types/internal-preact.d.ts
internal-preact.d.ts still imports/uses BackgroundSnapshotInstance; src exports BackgroundDOM — update the import and the vnode typing:
-import type { BackgroundSnapshotInstance, setupDom } from '../src/backgroundSnapshot.ts';
+import type { BackgroundDOM, setupDom } from '../src/backgroundSnapshot.js';
@@
- /** vnode */
- vnode?(vnode: VNode & BackgroundSnapshotInstance): void;
+ /** vnode */
+ vnode?(vnode: VNode & BackgroundDOM): void;
setupDom?: typeof setupDom;If your build expects extensionless imports in .d.ts use '../src/backgroundSnapshot' instead of '.js'.
- Also search for and update remaining references to BackgroundSnapshotInstance (e.g. tests under packages/react/runtime/test/*).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type { BackgroundSnapshotInstance, setupDom } from '../src/backgroundSnapshot.ts'; | |
| import type { BackgroundDOM, setupDom } from '../src/backgroundSnapshot.js'; | |
| /** vnode */ | |
| vnode?(vnode: VNode & BackgroundDOM): void; | |
| setupDom?: typeof setupDom; |
🤖 Prompt for AI Agents
In packages/react/runtime/types/internal-preact.d.ts around lines 5 to 6, the
file imports BackgroundSnapshotInstance from '../src/backgroundSnapshot.ts' but
the source exports BackgroundDOM; update the import to import type {
BackgroundDOM, setupDom } from '../src/backgroundSnapshot' (use the
extensionless path), replace all type occurrences of BackgroundSnapshotInstance
with BackgroundDOM (including the vnode typing), and then search the repo (e.g.
packages/react/runtime/__test__/*) for remaining references to
BackgroundSnapshotInstance and update them to BackgroundDOM.
| backgroundSnapshotInstanceManager.clear(); | ||
| backgroundSnapshotInstanceManager.nextId = 0; | ||
| target.__root = new BackgroundSnapshotInstance('root'); | ||
| target.__root = setupDom({ type: 'root' }); |
There was a problem hiding this comment.
Remove remaining options.document usage and dead document shim
PR goal is to drop options.document. This file still sets it on thread switches and keeps a document shim; that’s inconsistent and can cause leakage across threads.
Apply:
@@
- function setupDocument(document) {
- document.createElement = function(type) {
- return new SnapshotInstance(type);
- };
- document.createElementNS = function(_ns, type) {
- return new SnapshotInstance(type);
- };
- document.createTextNode = function(text) {
- const i = new SnapshotInstance(null);
- i.setAttribute(0, text);
- Object.defineProperty(i, 'data', {
- set(v) {
- i.setAttribute(0, v);
- },
- });
- return i;
- };
- return document;
- }
-
- target._document = setupDocument({});
+ // Removed legacy document shim; main-thread tests should use SnapshotInstance directly.
@@
globalThis.onSwitchedToMainThread = () => {
if (onSwitchedToMainThread) {
onSwitchedToMainThread();
}
setRoot(globalThis.__root);
- options.document = globalThis._document;
};
@@
globalThis.onSwitchedToBackgroundThread = () => {
if (onSwitchedToBackgroundThread) {
onSwitchedToBackgroundThread();
}
setRoot(globalThis.__root);
- options.document = globalThis._document;
};And drop the unused import if options is only used there.
-import { options } from 'preact';
+// import { options } from 'preact'; // no longer neededCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/react/testing-library/src/vitest-global-setup.js around line 90, the
code still assigns options.document (target.__root = setupDom({ type: 'root' })
implies a document shim is being set on thread switches) which reintroduces the
removed options.document API and keeps a dead document shim; remove the
assignment that sets options.document/document shim during thread switches,
delete the document shim code path entirely, and remove any imports that become
unused if options is only referenced for that assignment so no document state is
leaked across threads.
💡 Verification agent
🧩 Analysis chain
Verify no other references to options.document remain
Ensure full removal across repo.
🏁 Script executed:
#!/bin/bash
rg -n "options\.document" -g '!**/dist/**' -g '!**/build/**'Length of output: 148
🏁 Script executed:
#!/bin/bash
# Search for any remaining references to options.document across the repo, excluding dist and build folders
rg -n "options\.document" -g '*' -g '!**/dist/**' -g '!**/build/**'Length of output: 209
Remove remaining options.document references
packages/react/testing-library/src/vitest-global-setup.js — lines 133 and 141 contain options.document = globalThis._document; remove or update these assignments so options.document is fully removed across the repo.
🤖 Prompt for AI Agents
In packages/react/testing-library/src/vitest-global-setup.js around lines 90,
133 and 141, remove the leftover assignments that set options.document =
globalThis._document and any other references to options.document; update calls
and the options object so document is no longer passed or expected (e.g., stop
assigning or reading options.document, and remove related defaulting logic), and
ensure setupDom invocations and any downstream consumers use the global document
or explicitly passed nodes instead; run tests to confirm no remaining references
to options.document exist.
7818564 to
ad4c7d2
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. If this pull request is still relevant, please leave any comment (for example, "bump"). |
|
|
upupming
force-pushed
the
feat/simple-bsi
branch
2 times, most recently
from
ba4ea2d to
3c752c1
Compare
1 participant
We removed
options.documentand reusevnodetree asBackgroundSnapshotInstance(dom) treeSee preact changes at: https://github.com/hzy/preact/pull/2/files
Summary by CodeRabbit
New Features
Refactor
Tests
Chores
Checklist