Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/slick-pandas-flow.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@lynx-js/react": patch
---

Reuse vnode tree as dom tree in bts
Comment on lines +2 to +5
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Sep 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
"@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.

Copy link
Copy Markdown
Collaborator Author

@upupming upupming Sep 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai minor please, we are in 0.x stage

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

51 changes: 28 additions & 23 deletions benchmark/react/cases/004-various-update/index.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
// Copyright 2025 The Lynx Authors. All rights reserved.
// Licensed under the Apache License Version 2.0 that can be found in the
// LICENSE file in the root directory of this source tree.
/* eslint-disable @typescript-eslint/unbound-method */

import { root, useEffect, useState } from '@lynx-js/react';
import {
BackgroundSnapshotInstance,
SnapshotInstance,
} from '@lynx-js/react/internal';
import { SnapshotInstance, options } from '@lynx-js/react/internal';
import type { BackgroundDOM } from '@lynx-js/react/internal';
import type { CSSProperties, MainThread, NodesRef } from '@lynx-js/types';

import { hook, isMainThread } from '../../src/hook.js';
Expand Down Expand Up @@ -89,26 +88,32 @@ if (typeof Codspeed !== 'undefined' && __MAIN_THREAD__) {

if (typeof Codspeed !== 'undefined' && __BACKGROUND__) {
hook(
BackgroundSnapshotInstance.prototype,
'setAttribute',
function(this: BackgroundSnapshotInstance, old, 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 = old!.call(this, key, value);
Codspeed.stopBenchmark();
Codspeed.setExecutedBenchmark(
`${__REPO_FILEPATH__}::${__webpack_chunkname__}-setAttribute__BatchedValues`,
);
return ret;
}
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,
);
Comment on lines +91 to 117
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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.

}

Expand Down
6 changes: 1 addition & 5 deletions packages/react/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,6 @@
"lazy": "./runtime/lazy/internal.js",
"default": "./runtime/lib/internal.js"
},
"./internal/document": {
"types": "./runtime/lib/document.d.ts",
"default": "./runtime/lib/document.js"
},
"./jsx-runtime": {
"types": "./runtime/jsx-runtime/index.d.ts",
"lazy": "./runtime/lazy/jsx-runtime.js",
Expand Down Expand Up @@ -174,7 +170,7 @@
"api-extractor": "api-extractor run --verbose"
},
"dependencies": {
"preact": "npm:@lynx-js/internal-preact@10.28.4-ee7bb26"
"preact": "https://pkg.pr.new/lynx-family/internal-preact/preact@576a34a"
},
"devDependencies": {
"@lynx-js/types": "3.7.0",
Expand Down
13 changes: 8 additions & 5 deletions packages/react/runtime/__test__/compat.test.jsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest';

import { elementTree } from './utils/nativeMethod';
import { backgroundSnapshotInstanceManager, setupPage, snapshotInstanceManager } from '../src/snapshot';
import {
backgroundSnapshotInstanceManager,
setupPage,
SnapshotInstance,
snapshotInstanceManager,
} from '../src/snapshot';
import { ComponentFromReactRuntime, wrapWithLynxComponent } from '../src/compat/lynxComponent';
import { setupDocument } from '../src/document';
import { Fragment, render } from 'preact';
import { Fragment, options, render } from 'preact';
import { globalEnvManager } from './utils/envManager';

const HOLE = null;
Expand All @@ -13,7 +17,6 @@ let scratch;

beforeAll(() => {
globalEnvManager.switchToMainThread();
setupDocument();
setupPage(__CreatePage('0', 0));
});

Expand All @@ -22,7 +25,7 @@ beforeEach(() => {
backgroundSnapshotInstanceManager.nextId = 0;
snapshotInstanceManager.clear();
snapshotInstanceManager.nextId = 0;
scratch = document.createElement('root');
scratch = new SnapshotInstance('root');
});

afterEach(() => {
Expand Down
19 changes: 13 additions & 6 deletions packages/react/runtime/__test__/compat/initData.test.jsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import { Component, render } from 'preact';
import { Component, render, options } from 'preact';
import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest';
import { elementTree, waitSchedule } from '../utils/nativeMethod';
import { BackgroundSnapshotInstance } from '../../src/backgroundSnapshot';
import { setupBackgroundDocument } from '../../src/document';
import { backgroundSnapshotInstanceManager, setupPage, SnapshotInstance } from '../../src/snapshot';
import { backgroundSnapshotInstanceToJSON } from '../utils/debug';
import { useState } from 'preact/compat';
Expand All @@ -11,22 +10,30 @@ import { globalEnvManager } from '../utils/envManager';

/** @type {SnapshotInstance} */
let scratch;
let switchToBackground = globalEnvManager.switchToBackground.bind(globalEnvManager);

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;
};
};
Comment on lines +18 to +26
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


globalEnvManager.switchToBackground();
});

afterAll(() => {
delete BackgroundSnapshotInstance.prototype.toJSON;
globalEnvManager.switchToBackground = switchToBackground;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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.

});

beforeEach(() => {
scratch = document.createElement('root');
scratch = options.setupDom({ type: 'root' });
lynx.__initData = {};
});

Expand Down
Loading