Skip to content

fix(react/runtime): Accelerate the timing of effect and ref triggering#652

Merged
Yradex merged 9 commits intolynx-family:mainfrom
Yradex:lifecycle/ref-refactor-2
May 9, 2025
Merged

fix(react/runtime): Accelerate the timing of effect and ref triggering#652
Yradex merged 9 commits intolynx-family:mainfrom
Yradex:lifecycle/ref-refactor-2

Conversation

@Yradex
Copy link
Collaborator

@Yradex Yradex commented Apr 28, 2025

Summary

This PR fixes #405 by refactoring the ref and effect system to use Preact's ref implementation and accelerating the timing:

  • Fixed effect hook variable capture issue where stale values might be captured in closure.
  • Ensured ref and useEffect() side effects are now guaranteed to execute before event triggers.

Breaking Changes:

  • The execution timing of ref and useEffect() side effects has been moved forward. These effects will now execute before hydration is complete, rather than waiting for the main thread update to complete.
  • For components inside <list />, ref callbacks will now be triggered during background thread rendering, regardless of component visibility. If your code depends on component visibility timing, use main-thread:ref instead of regular ref.

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

@changeset-bot
Copy link

changeset-bot bot commented Apr 28, 2025

🦋 Changeset detected

Latest commit: 67de64e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lynx-js/react Minor

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

@codecov
Copy link

codecov bot commented Apr 28, 2025

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
2782 1 2781 104
View the top 2 failed test(s) by shortest run time
react.spec.ts::reactlynx3 testselementsx-inputbasic-element-x-input-bindconfirm
Stack Traces | 6.2s run time
react.spec.ts:2105:7 basic-element-x-input-bindconfirm
web-core.test.ts::web core testscreateJSObjectDestructionObserver
Stack Traces | 150s run time
web-core.test.ts:204:3 createJSObjectDestructionObserver

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@codecov
Copy link

codecov bot commented Apr 28, 2025

Bundle Report

Changes will increase total bundle size by 3.05kB (0.48%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
@lynx-js/example-react-lynx-cjs 320.84kB 3.05kB (0.96%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: @lynx-js/example-react-lynx-cjs

Assets Changed:

Asset Name Size Change Total Size Change (%)
main.lynx.bundle 1.54kB 89.68kB 1.75%
.rspeedy/main/main-thread.js 383 bytes 40.44kB 0.96%
.rspeedy/main/background.*.js 1.12kB 40.16kB 2.86%

@codspeed-hq
Copy link

codspeed-hq bot commented Apr 28, 2025

CodSpeed Performance Report

Merging #652 will not alter performance

Comparing Yradex:lifecycle/ref-refactor-2 (67de64e) with main (d90d91f)

Summary

✅ 1 untouched benchmarks

@Yradex Yradex requested a review from Copilot April 28, 2025 12:40
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request aims to accelerate the timing of effect and ref triggering in the React runtime by removing and refactoring parts of the ref update logic and related lifecycle event calls. Key changes include:

  • Removing calls to commitMainThreadPatchUpdate in list.ts and updateMainThread.
  • Removing and updating ref‐related constants and ref patch handling in lifecycle and snapshot modules.
  • Replacing useLayoutEffect with useEffect in hooks to adjust timing.

Reviewed Changes

Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/react/runtime/src/list.ts Removed calls to commitMainThreadPatchUpdate to accelerate lifecycle updates.
packages/react/runtime/src/lifecycleConstant.ts Removed the ref constant to reflect updated ref handling.
packages/react/runtime/src/lifecycle/ref/delay.ts Added a new mechanism for handling delayed UI operations with signals.
packages/react/runtime/src/lifecycle/patch/updateMainThread.ts Updated patch update logic by removing legacy ref patch updating.
packages/react/runtime/src/hooks/react.ts Replaced useLayoutEffect with useEffect to improve effect timing.
Various test files Updated snapshots and test expectations to align with the changes in ref patch handling.
Comments suppressed due to low confidence (3)

packages/react/runtime/src/list.ts:320

  • Confirm that removing the call to commitMainThreadPatchUpdate does not inadvertently delay or skip necessary lifecycle event propagation, especially regarding ref triggering.
commitMainThreadPatchUpdate(undefined);

packages/react/runtime/src/lifecycleConstant.ts:7

  • Ensure that the removal of the 'ref' constant is intentional and that any downstream logic relying on this constant has been appropriately updated.
public static readonly ref = 'rLynxRef';

packages/react/runtime/src/hooks/react.ts:32

  • Confirm that substituting useLayoutEffect with useEffect does not introduce unintended side effects in components that depend on synchronous layout updates.
return usePreactEffect(effect, deps);

@Yradex Yradex force-pushed the lifecycle/ref-refactor-2 branch from c1214d0 to 605a3a6 Compare April 29, 2025 06:43
@Yradex Yradex marked this pull request as ready for review April 29, 2025 07:19
@Yradex Yradex requested review from colinaaa and hzy April 29, 2025 07:20
colinaaa
colinaaa previously approved these changes Apr 29, 2025
Copy link
Collaborator

@colinaaa colinaaa left a comment

Choose a reason for hiding this comment

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

LGTM!

@Yradex Yradex force-pushed the lifecycle/ref-refactor-2 branch 3 times, most recently from 9d3305b to 381e2a3 Compare April 30, 2025 03:14
colinaaa
colinaaa previously approved these changes Apr 30, 2025
Yradex and others added 8 commits May 9, 2025 14:08
There is no need to detect if the type paramenter of `Parameters<>` is
function.

```diff
-    args: Parameters<NodesRef[K] extends (...args: any[]) => any ? NodesRef[K] : never>,
+    args: Parameters<NodesRef[K]>,
```
Add name for tuple.

```diff
-function applyRef(ref: Ref, value: null | [number, number]): void {
+function applyRef(ref: Ref, value: null | [snapshotInstanceId: number, expIndex: number]): void {
```
Signed-off-by: Yradex <11014207+Yradex@users.noreply.github.com>
@Yradex Yradex force-pushed the lifecycle/ref-refactor-2 branch from 381e2a3 to e5ca8fa Compare May 9, 2025 06:10
@Yradex Yradex added this pull request to the merge queue May 9, 2025
Merged via the queue into lynx-family:main with commit aba30cb May 9, 2025
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: useEffect() callbacks sometimes run later than expected

3 participants