fix(web): onNativeModulesCall should use promise to return ret #1457
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: a62cd02 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 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 |
Contributor
📝 WalkthroughWalkthroughA patch update for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/web-platform/web-core/src/apis/LynxView.ts (1)
205-212: Clear the cached calls array after processing to prevent memory leak.The cached native module calls are processed but never cleared from the array, which could lead to memory accumulation if the handler is reassigned multiple times.
Apply this diff to clear the array after processing:
set onNativeModulesCall(handler: NativeModulesCall) { this.#onNativeModulesCall = handler; if (this.#cachedNativeModulesCall) { for (const callInfo of this.#cachedNativeModulesCall) { callInfo.resolve(handler.apply(undefined, callInfo.args)); } + this.#cachedNativeModulesCall = []; } }Additionally, consider wrapping the handler call in a try-catch to handle potential exceptions:
set onNativeModulesCall(handler: NativeModulesCall) { this.#onNativeModulesCall = handler; if (this.#cachedNativeModulesCall) { for (const callInfo of this.#cachedNativeModulesCall) { - callInfo.resolve(handler.apply(undefined, callInfo.args)); + try { + callInfo.resolve(handler.apply(undefined, callInfo.args)); + } catch (error) { + console.error('Error in onNativeModulesCall handler:', error); + callInfo.resolve(undefined); + } } + this.#cachedNativeModulesCall = []; } }
🧹 Nitpick comments (1)
packages/web-platform/web-tests/shell-project/index.ts (1)
36-56: Eliminate code duplication in callback definition.The same callback function is defined identically in both branches of the conditional. This creates unnecessary duplication and maintenance burden.
Refactor to define the callback once and reuse it:
+ const nativeModulesCallback = (name, data, moduleName) => { + if (name === 'getColor' && moduleName === 'CustomModule') { + return data.color; + } + if (name === 'getColor' && moduleName === 'bridge') { + return data.color; + } + }; + if (casename === 'api-nativemodules-call-delay') { setTimeout(() => { - lynxView.onNativeModulesCall = (name, data, moduleName) => { - if (name === 'getColor' && moduleName === 'CustomModule') { - return data.color; - } - if (name === 'getColor' && moduleName === 'bridge') { - return data.color; - } - }; + lynxView.onNativeModulesCall = nativeModulesCallback; }, 2500); } else { - lynxView.onNativeModulesCall = (name, data, moduleName) => { - if (name === 'getColor' && moduleName === 'CustomModule') { - return data.color; - } - if (name === 'getColor' && moduleName === 'bridge') { - return data.color; - } - }; + lynxView.onNativeModulesCall = nativeModulesCallback; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/old-squids-spend.md(1 hunks)packages/web-platform/web-core/src/apis/LynxView.ts(3 hunks)packages/web-platform/web-tests/shell-project/index.ts(1 hunks)packages/web-platform/web-tests/tests/react.spec.ts(1 hunks)packages/web-platform/web-tests/tests/react/api-nativemodules-call-delay/index.jsx(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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#1454
File: pnpm-workspace.yaml:46-46
Timestamp: 2025-08-07T04:00:59.627Z
Learning: In the lynx-family/lynx-stack repository, the webpack patch (patches/webpack5.101.0.patch) was created to fix issues with webpack5.99.9 but only takes effect on webpack5.100.0 and later versions. The patchedDependencies entry should use "webpack@^5.100.0" to ensure the patch applies to the correct version range.
📚 Learning: in the lynx-family/lynx-stack repository, the webpack patch (patches/[email protected]) was crea...
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1454
File: pnpm-workspace.yaml:46-46
Timestamp: 2025-08-07T04:00:59.627Z
Learning: In the lynx-family/lynx-stack repository, the webpack patch (patches/webpack5.101.0.patch) was created to fix issues with webpack5.99.9 but only takes effect on webpack5.100.0 and later versions. The patchedDependencies entry should use "webpack@^5.100.0" to ensure the patch applies to the correct version range.
Applied to files:
.changeset/old-squids-spend.md
📚 Learning: in the lynx-family/lynx-stack repository, changesets are only required for meaningful changes to end...
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/old-squids-spend.mdpackages/web-platform/web-core/src/apis/LynxView.ts
📚 Learning: in the lynx-family/lynx-stack repository, ci checks require changesets when files matching the patte...
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/old-squids-spend.md
📚 Learning: in the lynx-family/lynx-stack repository, the file `packages/react/testing-library/src/vitest.config...
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1453
File: vitest.config.ts:49-61
Timestamp: 2025-08-06T13:28:57.139Z
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/web-platform/web-tests/tests/react.spec.tspackages/web-platform/web-tests/shell-project/index.ts
📚 Learning: in the lynx-family/lynx-stack repository, the file `packages/rspeedy/create-rspeedy/template-react-v...
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1453
File: vitest.config.ts:49-61
Timestamp: 2025-08-06T13:28:57.139Z
Learning: In the lynx-family/lynx-stack repository, the file `packages/rspeedy/create-rspeedy/template-react-vitest-rltl-js/vitest.config.js` is a template file for scaffolding new Rspeedy projects, not a test configuration that should be included in the main vitest projects array.
Applied to files:
packages/web-platform/web-tests/tests/react.spec.ts
📚 Learning: in the lynx-stack ssr implementation, each createlynxview instance is used to render once and then d...
Learnt from: PupilTong
PR: lynx-family/lynx-stack#1029
File: packages/web-platform/web-core-server/src/createLynxView.ts:0-0
Timestamp: 2025-07-16T06:26:22.230Z
Learning: In the lynx-stack SSR implementation, each createLynxView instance is used to render once and then discarded. There's no reuse of the same instance for multiple renders, so event arrays and other state don't need to be cleared between renders.
Applied to files:
packages/web-platform/web-core/src/apis/LynxView.tspackages/web-platform/web-tests/shell-project/index.ts
📚 Learning: in the lynx-stack codebase, css selectors in ssr hydration are generated by their own packages, ensu...
Learnt from: PupilTong
PR: lynx-family/lynx-stack#1029
File: packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts:95-99
Timestamp: 2025-07-16T06:28:26.463Z
Learning: In the lynx-stack codebase, CSS selectors in SSR hydration are generated by their own packages, ensuring a predictable format that makes simple string manipulation safe and preferable over regex for performance reasons.
Applied to files:
packages/web-platform/web-tests/shell-project/index.ts
📚 Learning: in the lynx-stack codebase, css strings produced by `gencsscontent` are considered trusted developer...
Learnt from: PupilTong
PR: lynx-family/lynx-stack#1029
File: packages/web-platform/web-mainthread-apis/src/createMainThreadGlobalThis.ts:214-217
Timestamp: 2025-07-16T06:25:41.055Z
Learning: In the lynx-stack codebase, CSS strings produced by `genCssContent` are considered trusted developer input, so additional sanitization/escaping is not required.
Applied to files:
packages/web-platform/web-tests/shell-project/index.ts
📚 Learning: in the lynx-stack codebase, pupiltong prefers the "let it crash" approach over defensive null safety...
Learnt from: PupilTong
PR: lynx-family/lynx-stack#1292
File: packages/web-platform/web-core-server/src/createLynxView.ts:144-151
Timestamp: 2025-07-15T10:00:56.154Z
Learning: In the lynx-stack codebase, PupilTong prefers the "let it crash" approach over defensive null safety checks when the condition should never occur in normal operation. This applies to cases like the `element.getAttribute(lynxUniqueIdAttribute)!` call in SSR event capture where the attribute is expected to always be present.
Applied to files:
packages/web-platform/web-tests/shell-project/index.ts
🧬 Code Graph Analysis (1)
packages/web-platform/web-tests/shell-project/index.ts (1)
packages/web-platform/web-tests/server.js (1)
lynxView(16-33)
🔇 Additional comments (6)
.changeset/old-squids-spend.md (1)
1-6: LGTM! Proper changeset for user-facing bugfix.The changeset correctly documents a patch-level fix for a legitimate issue where NativeModules execution results could be undefined due to timing issues with delayed callbacks. This aligns with the repository's requirement for changesets on meaningful user-facing changes.
packages/web-platform/web-tests/shell-project/index.ts (1)
36-46: Test setup correctly implements delayed callback scenario.The conditional delay for the 'api-nativemodules-call-delay' test case effectively simulates the timing issue described in the changeset, where the
onNativeModulesCallcallback is not immediately available during mounting. The 2500ms delay provides sufficient time to test the caching mechanism.packages/web-platform/web-tests/tests/react.spec.ts (1)
551-559: LGTM! Well-implemented test for delayed native module callbacks.The test case appropriately validates the fix for delayed
onNativeModulesCallcallbacks by:
- Using a longer wait time (3000ms) to simulate the delayed callback scenario
- Following the established test pattern and naming convention
- Properly asserting the expected outcome (green background color)
The implementation aligns well with the PR objectives and complements the existing test suite.
packages/web-platform/web-tests/tests/react/api-nativemodules-call-delay/index.jsx (1)
1-26: LGTM! Well-structured test for delayed native module calls.The test component correctly demonstrates the native module call with callback pattern, which will help verify that the promise-based caching mechanism works properly when the handler is delayed.
packages/web-platform/web-core/src/apis/LynxView.ts (2)
191-196: Good implementation of promise-based caching structure.The change from tuples to objects with resolve callbacks enables proper promise-based handling of delayed native module calls.
449-456: Promise-based return correctly implements deferred resolution.The implementation properly returns a Promise when the handler is not yet set, enabling the caller to await the result that will be resolved once the handler is assigned.
PupilTong
reviewed
Aug 7, 2025
Sherry-hue
force-pushed
the
fix/native-modules-delay
branch
from
cf9e084 to
40478a8
Compare
CodSpeed Performance ReportMerging #1457 will not alter performanceComparing Summary
|
Web Explorer#3843 Bundle Size — 342.66KiB (~+0.01%).a62cd02(current) vs 5378779 main#3834(baseline) Bundle metrics
|
| Current #3843 |
Baseline #3834 |
|
|---|---|---|
 Initial JS |
142.35KiB(+0.01%) |
142.33KiB |
 Initial CSS |
31.84KiB |
31.84KiB |
 Cache Invalidation |
41.54% |
0% |
| Chunks |
7 |
7 |
| Assets |
7 |
7 |
| Modules |
211(-0.47%) |
212 |
| Duplicate Modules |
17 |
17 |
| Duplicate Code |
3.96% |
3.96% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #3843 |
Baseline #3834 |
|
|---|---|---|
| JS |
227.87KiB (~+0.01%) |
227.85KiB |
| Other |
82.95KiB |
82.95KiB |
| CSS |
31.84KiB |
31.84KiB |
Bundle analysis report Branch Sherry-hue:fix/native-modules-de... Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#3849 Bundle Size — 235.26KiB (0%).a62cd02(current) vs 5378779 main#3839(baseline) Bundle metrics
|
| Current #3849 |
Baseline #3839 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
159 |
159 |
| Duplicate Modules |
64 |
64 |
| Duplicate Code |
45.81% |
45.81% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #3849 |
Baseline #3839 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
89.5KiB |
89.5KiB |
Bundle analysis report Branch Sherry-hue:fix/native-modules-de... Project dashboard
Generated by RelativeCI Documentation Report issue
Co-authored-by: PupilTong <[email protected]> Signed-off-by: Sherry <[email protected]>
Co-authored-by: PupilTong <[email protected]> Signed-off-by: Sherry <[email protected]>
Sherry-hue
force-pushed
the
fix/native-modules-delay
branch
from
40478a8 to
a62cd02
Compare
PupilTong
approved these changes
Aug 7, 2025
3 tasks
colinaaa
pushed a commit
that referenced
this pull request
Aug 9, 2025
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/[email protected] ### Patch Changes - Supports `recyclable` attribute in `<list-item>` to control whether the list item is recyclable. The `recyclable` attribute depends on Lynx Engine 3.4 or later. ([#1388](#1388)) ```jsx <list-item recyclable={false} /> ``` - feat: Support using a host element as direct child of Suspense ([#1455](#1455)) - Add profile in production build: ([#1336](#1336)) 1. `diff:__COMPONENT_NAME__`: how long ReactLynx diff took. 2. `render:__COMPONENT_NAME__`: how long your render function took. 3. `setState`: an instant trace event, indicate when your setState was called. NOTE: `__COMPONENT_NAME__` may be unreadable when minified, setting `displayName` may help. - Add `onBackgroundSnapshotInstanceUpdateId` event on dev for Preact Devtools to keep the correct snapshotInstanceId info. ([#1173](#1173)) - fix: Prevent error when spreading component props onto an element ([#1459](#1459)) - fix: Correctly check for the existence of background functions in MTS ([#1416](#1416)) ```ts function handleTap() { "main thread"; // The following check always returned false before this fix if (myHandleTap) { runOnBackground(myHandleTap)(); } } ``` ## @lynx-js/[email protected] ### Patch Changes - Remove the experimental `provider` option. ([#1432](#1432)) - Add `output.filename.wasm` and `output.filename.assets` options. ([#1449](#1449)) - fix deno compatibility ([#1412](#1412)) - Should call the `api.onCloseBuild` hook after the build finished. ([#1446](#1446)) - Bump Rsbuild v1.4.15. ([#1423](#1423)) - Support using function in `output.filename.*`. ([#1449](#1449)) ## [email protected] ### Patch Changes - Support ESLint for ReactLynx templates ([#1274](#1274)) ## @lynx-js/[email protected] ### Patch Changes - Updated dependencies \[[`c8ce6aa`](c8ce6aa)]: - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - Fix the `Package subpath './compat' is not defined by "exports"` error. ([#1460](#1460)) ## @lynx-js/[email protected] ### Patch Changes - Fix `GlobalEventEmitter` type definition, the `emit(eventName: string, data: unknown)` function should recevie an array typed `data` and pass as param list of listeners. ([#1479](#1479)) ## @lynx-js/[email protected] ### Patch Changes - fix: load main-thread chunk in ESM format ([#1437](#1437)) See [nodejs/node#59362](nodejs/node#59362) for more details. - feat: support path() for `createQuerySelector` ([#1456](#1456)) - Added `getPathInfo` API to `NativeApp` and its cross-thread handler for retrieving the path from a DOM node to the root. - Implemented endpoint and handler registration in both background and UI threads. - Implemented `nativeApp.getPathInfo()` - Updated dependencies \[]: - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - fix: load main-thread chunk in ESM format ([#1437](#1437)) See [nodejs/node#59362](nodejs/node#59362) for more details. - feat: support path() for `createQuerySelector` ([#1456](#1456)) - Added `getPathInfo` API to `NativeApp` and its cross-thread handler for retrieving the path from a DOM node to the root. - Implemented endpoint and handler registration in both background and UI threads. - Implemented `nativeApp.getPathInfo()` - fix: when `onNativeModulesCall` is delayed in mounting, the NativeModules execution result may be undefined. ([#1457](#1457)) - fix: `onNativeModulesCall` && `onNapiModulesCall` use getter to get. ([#1466](#1466)) - Updated dependencies \[[`29434ae`](29434ae), [`fb7096b`](fb7096b)]: - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - fix: load main-thread chunk in ESM format ([#1437](#1437)) See [nodejs/node#59362](nodejs/node#59362) for more details. ## @lynx-js/[email protected] ### Patch Changes - feat: add autocomplete attribute support for x-input component ([#1444](#1444)) Implements autocomplete attribute forwarding from the x-input custom element to the internal HTML input element in the shadow DOM. This enables standard browser autocomplete functionality for x-input elements. - Add referrerpolicy attribute support to x-image web component ([#1420](#1420)) - Updated dependencies \[]: - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - fix: load main-thread chunk in ESM format ([#1437](#1437)) See [nodejs/node#59362](nodejs/node#59362) for more details. - Updated dependencies \[[`29434ae`](29434ae), [`fb7096b`](fb7096b)]: - @lynx-js/[email protected] - @lynx-js/[email protected] ## @lynx-js/[email protected] ### Patch Changes - feat: support path() for `createQuerySelector` ([#1456](#1456)) - Added `getPathInfo` API to `NativeApp` and its cross-thread handler for retrieving the path from a DOM node to the root. - Implemented endpoint and handler registration in both background and UI threads. - Implemented `nativeApp.getPathInfo()` - Updated dependencies \[[`29434ae`](29434ae), [`fb7096b`](fb7096b)]: - @lynx-js/[email protected] - @lynx-js/[email protected] - @lynx-js/[email protected] ## [email protected] ## @lynx-js/[email protected] ## @lynx-js/[email protected] ## @lynx-js/[email protected] Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores
Checklist