fix(react): MTF variable capture in class components#2197
fix(react): MTF variable capture in class components#2197gaoachao merged 2 commits intolynx-family:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 9d3d256 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds a patch changeset and three accessor methods to the identifier extractor; refactors class-member worklet transformation to explicitly handle class methods and class properties (including converting fields to getters for JS targets when captures are present) and updates/adds snapshot tests for these code paths. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/react/transform/crates/swc_plugin_worklet/lib.rs`:
- Around line 138-149: Clippy flags the redundant .into() call on the BlockStmt
construction assigned to getter_fn.body; remove the unnecessary conversion so
you construct the BlockStmt and provide it in the expected form (i.e., avoid
calling .into() on the BlockStmt literal) while keeping the ReturnStmt.into()
as-is; update the expression that assigns getter_fn.body (the BlockStmt { ...
}.into() portion) to directly use the BlockStmt in the type expected by
getter_fn.body, referencing GetterFn.body, BlockStmt, ReturnStmt, and
worklet_object_expr to locate the code.
- Around line 256-267: The BlockStmt construction used for the class property
getter is being unnecessarily converted with an extra .into() causing a clippy
lint; locate the body: Some(BlockStmt { ... .stmts: vec![ReturnStmt { ... arg:
Some(worklet_object_expr), } .into()], } .into(),) expression and remove the
redundant .into() on the BlockStmt (leave the BlockStmt value or a single
appropriate conversion), so the BlockStmt is not double-converted while keeping
the ReturnStmt and worklet_object_expr usage intact.
Merging this PR will degrade performance by 8.72%
Performance Changes
Comparing Footnotes
|
Web Explorer#7548 Bundle Size — 383.72KiB (0%).9d3d256(current) vs f9f0987 main#7546(baseline) Bundle metrics
|
| Current #7548 |
Baseline #7546 |
|
|---|---|---|
154.88KiB |
154.88KiB |
|
35.05KiB |
35.05KiB |
|
0% |
0% |
|
8 |
8 |
|
8 |
8 |
|
237 |
237 |
|
16 |
16 |
|
2.99% |
2.99% |
|
4 |
4 |
|
0 |
0 |
Bundle size by type no changes
| Current #7548 |
Baseline #7546 |
|
|---|---|---|
252.83KiB |
252.83KiB |
|
95.85KiB |
95.85KiB |
|
35.05KiB |
35.05KiB |
Bundle analysis report Branch Yradex:wt/fix-mtf-component-not-... Project dashboard
Generated by RelativeCI Documentation Report issue
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @lynx-js/motion@0.0.3 ### Patch Changes - Fix an issue that motion/mini will accidentally imports full version, causing compiling error ([#2204](#2204)) ## @lynx-js/react@0.116.3 ### Patch Changes - fix: remove `lynx.createSelectorQuery` deprecated warning in production ([#2195](#2195)) - Add a DEV-only guard that detects MainThread flush loops caused by re-entrant MTS handlers. ([#2159](#2159)) This typically happens when a MainThread handler (e.g. event callback or `MainThreadRef`) performs UI mutations (like `Element.setStyleProperty`, `setStyleProperties`, `setAttribute`, or `invoke`) that synchronously trigger a flush which re-enters the handler again. - Avoid DEV_ONLY_SetSnapshotEntryName on standalone lazy bundle. ([#2184](#2184)) - Add alog and trace for BTS event handlers. ([#2102](#2102)) - fix: Main thread functions cannot access properties on `this` before hydration completes. ([#2194](#2194)) This fixes the `cannot convert to object` error. - Remove element api calls alog by default, and only enable it when `__ALOG_ELEMENT_API__` is defined to `true` or environment variable `REACT_ALOG_ELEMENT_API` is set to `true`. ([#2192](#2192)) - fix: captured variables in main thread functions within class components do not update correctly ([#2197](#2197)) ## @lynx-js/rspeedy@0.13.4 ### Patch Changes - Bump ts-blank-space v0.7.0 ([#2238](#2238)) - Bump Rsbuild v1.7.3 with Rspack v1.7.5. ([#2189](#2189)) - Updated dependencies \[]: - @lynx-js/web-rsbuild-server-middleware@0.19.8 ## @lynx-js/qrcode-rsbuild-plugin@0.4.5 ### Patch Changes - Only register console shortcuts when running in TTY environments ([#2202](#2202)) ## @lynx-js/react-rsbuild-plugin@0.12.8 ### Patch Changes - Updated dependencies \[[`4240138`](4240138)]: - @lynx-js/react-webpack-plugin@0.7.4 - @lynx-js/react-alias-rsbuild-plugin@0.12.8 - @lynx-js/use-sync-external-store@1.5.0 - @lynx-js/react-refresh-webpack-plugin@0.3.4 - @lynx-js/template-webpack-plugin@0.10.3 ## @lynx-js/testing-environment@0.1.11 ### Patch Changes - Remove element api calls alog by default, and only enable it when `__ALOG_ELEMENT_API__` is defined to `true` or environment variable `REACT_ALOG_ELEMENT_API` is set to `true`. ([#2192](#2192)) ## @lynx-js/web-constants@0.19.8 ### Patch Changes - Updated dependencies \[]: - @lynx-js/web-worker-rpc@0.19.8 ## @lynx-js/web-core@0.19.8 ### Patch Changes - fix: avoid error when LynxView is removed immediately after connected ([#2182](#2182)) - Updated dependencies \[]: - @lynx-js/web-constants@0.19.8 - @lynx-js/web-mainthread-apis@0.19.8 - @lynx-js/web-worker-rpc@0.19.8 - @lynx-js/web-worker-runtime@0.19.8 ## @lynx-js/web-core-wasm@0.0.3 ### Patch Changes - Updated dependencies \[[`e0972ef`](e0972ef), [`3bc017e`](3bc017e), [`e2d349e`](e2d349e), [`d924b6a`](d924b6a)]: - @lynx-js/web-elements@0.11.2 - @lynx-js/web-worker-rpc@0.19.8 ## @lynx-js/web-elements@0.11.2 ### Patch Changes - Add scrollHeight/scrollWidth getters to XList. ([#2156](#2156)) - Inherit padding styles for x-input elements. ([#2199](#2199)) - Remove the default lazy-loading attribute from x-image elements. ([#2186](#2186)) - Fix x-input number type forwarding to the inner input element. ([#2193](#2193)) ## @lynx-js/web-mainthread-apis@0.19.8 ### Patch Changes - Updated dependencies \[]: - @lynx-js/web-constants@0.19.8 ## @lynx-js/web-worker-runtime@0.19.8 ### Patch Changes - Updated dependencies \[]: - @lynx-js/web-constants@0.19.8 - @lynx-js/web-mainthread-apis@0.19.8 - @lynx-js/web-worker-rpc@0.19.8 ## @lynx-js/react-webpack-plugin@0.7.4 ### Patch Changes - Remove element api calls alog by default, and only enable it when `__ALOG_ELEMENT_API__` is defined to `true` or environment variable `REACT_ALOG_ELEMENT_API` is set to `true`. ([#2192](#2192)) ## create-rspeedy@0.13.4 ## @lynx-js/react-alias-rsbuild-plugin@0.12.8 ## upgrade-rspeedy@0.13.4 ## @lynx-js/web-core-server@0.19.8 ## @lynx-js/web-rsbuild-server-middleware@0.19.8 ## @lynx-js/web-worker-rpc@0.19.8 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit
Problem
Main-thread worklets inside class components could read stale captured values (e.g.
this.ainonTapLepus), because:TransformTarget::JS, worklet ctx objects are later transformed into functions and invoked withthisbound to the ctx object, not the component instance.onTapLepus = { ... a: this.a }, captured values (this.a, or_cfor lexical/module vars) are computed once during construction, then remain frozen on the ctx.This breaks cases like
onTapLepusreading the latestthis.a/ updated module variables.What changed
swc_plugin_worklet: ForTransformTarget::JS, non-static class members that are worklets now emit a getter (get onTapLepus() { return { ... } }) when the worklet captures anything (_c/ extractedthis.xxx/_jsFn), so the ctx object is re-created on each access._c/this/_jsFncaptures exist.class A { onTapLepus(e) { "main thread"; ... } }class A { onTapLepus = (e) => { "main thread"; ... } }LEPUS/MIXEDbecause the lepus registration path writesthis["onTapLepus"] = ..., which can conflict with getter-only accessors.Tests
should_transform_in_class_jsshould_transform_in_class_property_jsshould_transform_in_class_js_capture_valuesshould_transform_in_class_property_js_capture_valuesHow to review
packages/react/transform/crates/swc_plugin_worklet/lib.rs:WorkletVisitor::visit_mut_class_member:get onTapLepus()onTapLepus = (...) => { ... }packages/react/transform/crates/swc_plugin_worklet/extract_ident.rs.packages/react/transform/crates/swc_plugin_worklet/tests/__swc_snapshots__/lib.rs/and confirm JS output now uses getter in the capture cases.LEPUSoutput is unchanged for class methods (still a class field worklet ctx object) to avoid runtime assignment issues.Checklist