fix(react): add global event unstable_onLazyBundleResponse
#655
Conversation
🦋 Changeset detectedLatest commit: d73d645 The changes in this PR will be included in the next version bump. 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 ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
📢 Thoughts on this report? Let us know! |
CodSpeed Performance ReportMerging #655 will not alter performanceComparing Summary
|
hzy
force-pushed
the
p/hzy/unstable_on_lazy_bundle_response
branch
3 times, most recently
from
43aa2e8 to
04ef850
Compare
hzy
force-pushed
the
p/hzy/unstable_on_lazy_bundle_response
branch
from
04ef850 to
d73d645
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"). |
WalkthroughThis change introduces a new experimental API and React hook for listening to lazy bundle responses in the Lynx environment. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant App as React App
participant LazyBundleResponseListener
participant LynxAPI
participant GlobalEventEmitter
App->>LazyBundleResponseListener: Mounts with onResponse callback
LazyBundleResponseListener->>LynxAPI: Calls experimental_addLazyBundleResponseListener
LynxAPI->>GlobalEventEmitter: Registers event listener for experimental_onLazyBundleResponse
GlobalEventEmitter-->>LynxAPI: Emits lazy bundle response event
LynxAPI-->>LazyBundleResponseListener: Invokes onResponse callback with response
LazyBundleResponseListener-->>App: App handles bundle response event
App->>LazyBundleResponseListener: Unmounts
LazyBundleResponseListener->>LynxAPI: Calls cleanup to remove listener
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/react/types/react.docs.d.ts (1)
3-6: Fix license header placement.The import statement should come after the license header, not before it. This breaks the standard file header pattern used throughout the codebase.
Apply this diff to fix the header order:
-// Licensed under the Apache License Version 2.0 that can be found in the -import type { ReactNode, ExoticComponent } from 'react'; - -// LICENSE file in the root directory of this source tree. +// Licensed under the Apache License Version 2.0 that can be found in the +// LICENSE file in the root directory of this source tree. + +import type { ReactNode, ExoticComponent } from 'react';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/fast-pandas-serve.md(1 hunks)examples/react/src/App.tsx(1 hunks)examples/react/src/index.tsx(1 hunks)packages/react/runtime/src/index.ts(2 hunks)packages/react/runtime/src/lynx-api.ts(1 hunks)packages/react/runtime/src/lynx/env.ts(1 hunks)packages/react/runtime/src/lynx/lazy-bundle.ts(4 hunks)packages/react/runtime/types/internal-preact.d.ts(1 hunks)packages/react/types/react.docs.d.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
packages/react/runtime/types/internal-preact.d.ts (1)
Learnt from: colinaaa
PR: #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.
packages/react/runtime/src/index.ts (1)
Learnt from: colinaaa
PR: #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.
packages/react/types/react.docs.d.ts (1)
Learnt from: colinaaa
PR: #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.
examples/react/src/index.tsx (1)
Learnt from: colinaaa
PR: #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.
packages/react/runtime/src/lynx/lazy-bundle.ts (2)
Learnt from: colinaaa
PR: #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.
Learnt from: PupilTong
PR: #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.
.changeset/fast-pandas-serve.md (1)
Learnt from: colinaaa
PR: #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.
🧬 Code Graph Analysis (2)
packages/react/types/react.docs.d.ts (2)
packages/react/runtime/src/lynx/lazy-bundle.ts (1)
LazyBundleResponseListener(56-92)packages/react/types/react.d.ts (2)
ExoticComponent(40-40)ReactNode(69-69)
packages/react/runtime/src/lynx/lazy-bundle.ts (3)
packages/react/runtime/src/index.ts (2)
LazyBundleResponseListener(87-87)Component(37-37)packages/react/types/react.docs.d.ts (2)
LazyBundleResponseListener(74-77)Component(72-72)packages/react/types/react.d.ts (1)
ComponentType(27-27)
🪛 GitHub Check: typos
packages/react/runtime/src/lynx/lazy-bundle.ts
[warning] 59-59:
"Fullfill" should be "Fulfill".
[warning] 67-67:
"Fullfill" should be "Fulfill".
[warning] 67-67:
"Fullfill" should be "Fulfill".
🪛 GitHub Actions: Test
packages/react/runtime/src/lynx/lazy-bundle.ts
[error] 59-59: Typo error: Fullfill should be Fulfill.
[error] 67-67: Typo error: Fullfill should be Fulfill at first occurrence on this line.
[error] 67-67: Typo error: Fullfill should be Fulfill at second occurrence on this line.
🔇 Additional comments (10)
packages/react/runtime/src/index.ts (1)
35-35: LGTM! Consistent import and export pattern.The addition of
LazyBundleResponseListenerto both the import statement and export list follows the established pattern in this module and properly exposes the new component through the public API.Also applies to: 87-87
examples/react/src/App.tsx (1)
8-8: LGTM! Export change supports lazy loading.The change from named export to default export is necessary to support the dynamic import syntax used in the lazy loading implementation in
examples/react/src/index.tsx.packages/react/runtime/types/internal-preact.d.ts (1)
23-24: LGTM! Parent reference enables vnode tree traversal.The addition of the
__property (parent VNode reference) to the VNode interface enables upward traversal of the component tree, which is essential for theLazyBundleResponseListenercomponent to locate itself in the vnode hierarchy during error handling.packages/react/types/react.docs.d.ts (1)
74-77: LGTM! Proper type declaration for the new component.The type declaration correctly defines
LazyBundleResponseListeneras anExoticComponentwith appropriate props:
- Optional
childrenprop following React patterns- Required
onResponsecallback with flexibleunknownparameter typeexamples/react/src/index.tsx (2)
5-7: LGTM! Simple utility for demonstration.The
sleepfunction provides a clean way to add artificial delay for demonstrating the lazy loading behavior.
12-16: LGTM! Proper demonstration of the new lazy bundle listener.The implementation correctly demonstrates the new functionality:
Suspenseboundary handles the loading stateLazyBundleResponseListenerwraps the lazy component to capture bundle responsesconsole.logprovides simple logging for demonstration purposespackages/react/runtime/src/lynx-api.ts (1)
372-420: Well-designed experimental API with comprehensive documentation.The API design follows good practices:
- Returns a cleanup function for proper resource management
- Comprehensive documentation with practical examples
- Proper experimental marking indicating API instability
- Clear guidance on React usage patterns including memoization and cleanup
packages/react/runtime/src/lynx/env.ts (1)
110-119: Correct implementation of the experimental API.The implementation properly:
- Conditionally registers listeners only in background contexts
- Uses the appropriate GlobalEventEmitter methods for add/remove operations
- Returns a cleanup function that removes the listener when called
- Provides a no-op fallback for non-background contexts
.changeset/fast-pandas-serve.md (1)
1-28: Well-documented changeset for the new feature.The changeset appropriately documents this new experimental API as a patch release with clear usage examples for both direct API usage and React hook integration.
packages/react/runtime/src/lynx/lazy-bundle.ts (1)
56-92: Well-structured component with proper cleanup integration.The component design properly integrates with Preact's component lifecycle and error handling system. The vnode tree traversal logic to find the listener component is correctly implemented.
| return new Promise(resolve => setTimeout(resolve, ms)); | ||
| } | ||
|
|
||
| const App = lazy(() => sleep(1000).then(() => import('./App.jsx'))); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the import path extension.
The lazy import references './App.jsx' but the actual file is App.tsx. Please verify that this is intentional and handled correctly by the build system.
🏁 Script executed:
#!/bin/bash
# Check if App.jsx exists or if this is a build system configuration
fd -e jsx -e tsx "App" examples/react/src/Length of output: 69
Fix the import path extension in examples/react/src/index.tsx
The dynamic import on line 9 references ./App.jsx, but the file is actually named App.tsx. Update the import to avoid module-not-found errors:
- const App = lazy(() => sleep(1000).then(() => import('./App.jsx')));
+ const App = lazy(() => sleep(1000).then(() => import('./App.tsx')));📝 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.
| const App = lazy(() => sleep(1000).then(() => import('./App.jsx'))); | |
| const App = lazy(() => sleep(1000).then(() => import('./App.tsx'))); |
🤖 Prompt for AI Agents
In examples/react/src/index.tsx at line 9, the dynamic import path incorrectly
uses './App.jsx' while the actual file is 'App.tsx'. Update the import statement
to use './App.tsx' to match the file extension and prevent module-not-found
errors.
| const responseMap: Map<Promise<any>, any> = /* @__PURE__ */ new Map(); | ||
|
|
||
| export const LazyBundleResponseListener: ComponentType<any> = /* @__PURE__ */ (function() { | ||
| class LazyBundleResponseListener extends Component<any> { | ||
| _onLazyBundleResponse(p: any): void { | ||
| const onFullfill = () => { | ||
| if (responseMap.has(p)) { | ||
| if (this.props.onResponse) { | ||
| const { code, data } = responseMap.get(p)!; | ||
| this.props.onResponse({ code, data }); | ||
| } | ||
| } | ||
| }; | ||
| p.then(onFullfill, onFullfill); | ||
| } |
There was a problem hiding this comment.
Address responseMap population issue.
The responseMap is checked but never populated, causing the response callback to never be triggered. Line 131 in loadLazyBundle has the population code commented out.
Either uncomment line 131 to populate the responseMap:
- // responseMap.set(p, result);
+ responseMap.set(p, result);Or modify the component to work without the responseMap if it's not needed:
_onLazyBundleResponse(p: any): void {
const onFulfill = () => {
- if (responseMap.has(p)) {
- if (this.props.onResponse) {
- const { code, data } = responseMap.get(p)!;
- this.props.onResponse({ code, data });
- }
- }
+ if (this.props.onResponse) {
+ // Call with the promise itself since responseMap is not populated
+ this.props.onResponse(p);
+ }
};
p.then(onFulfill, onFulfill);
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: typos
[warning] 59-59:
"Fullfill" should be "Fulfill".
[warning] 67-67:
"Fullfill" should be "Fulfill".
[warning] 67-67:
"Fullfill" should be "Fulfill".
🪛 GitHub Actions: Test
[error] 59-59: Typo error: Fullfill should be Fulfill.
[error] 67-67: Typo error: Fullfill should be Fulfill at first occurrence on this line.
[error] 67-67: Typo error: Fullfill should be Fulfill at second occurrence on this line.
🤖 Prompt for AI Agents
In packages/react/runtime/src/lynx/lazy-bundle.ts around lines 54 to 68, the
responseMap is checked for promises but never populated, so the response
callback never triggers. To fix this, either uncomment the code at line 131 in
loadLazyBundle that populates responseMap with the promise and its response
data, or refactor the LazyBundleResponseListener component to avoid relying on
responseMap by handling the promise responses directly without the map.
| const onFullfill = () => { | ||
| if (responseMap.has(p)) { | ||
| if (this.props.onResponse) { | ||
| const { code, data } = responseMap.get(p)!; | ||
| this.props.onResponse({ code, data }); | ||
| } | ||
| } | ||
| }; | ||
| p.then(onFullfill, onFullfill); |
There was a problem hiding this comment.
Fix typos in method name.
The method name contains typos that are causing pipeline failures.
Apply this diff to fix the typos:
- _onLazyBundleResponse(p: any): void {
- const onFullfill = () => {
- if (responseMap.has(p)) {
- if (this.props.onResponse) {
- const { code, data } = responseMap.get(p)!;
- this.props.onResponse({ code, data });
- }
- }
- };
- p.then(onFullfill, onFullfill);
- }
+ _onLazyBundleResponse(p: any): void {
+ const onFulfill = () => {
+ if (responseMap.has(p)) {
+ if (this.props.onResponse) {
+ const { code, data } = responseMap.get(p)!;
+ this.props.onResponse({ code, data });
+ }
+ }
+ };
+ p.then(onFulfill, onFulfill);
+ }📝 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.
| const onFullfill = () => { | |
| if (responseMap.has(p)) { | |
| if (this.props.onResponse) { | |
| const { code, data } = responseMap.get(p)!; | |
| this.props.onResponse({ code, data }); | |
| } | |
| } | |
| }; | |
| p.then(onFullfill, onFullfill); | |
| _onLazyBundleResponse(p: any): void { | |
| const onFulfill = () => { | |
| if (responseMap.has(p)) { | |
| if (this.props.onResponse) { | |
| const { code, data } = responseMap.get(p)!; | |
| this.props.onResponse({ code, data }); | |
| } | |
| } | |
| }; | |
| p.then(onFulfill, onFulfill); | |
| } |
🧰 Tools
🪛 GitHub Check: typos
[warning] 59-59:
"Fullfill" should be "Fulfill".
[warning] 67-67:
"Fullfill" should be "Fulfill".
[warning] 67-67:
"Fullfill" should be "Fulfill".
🪛 GitHub Actions: Test
[error] 59-59: Typo error: Fullfill should be Fulfill.
[error] 67-67: Typo error: Fullfill should be Fulfill at first occurrence on this line.
[error] 67-67: Typo error: Fullfill should be Fulfill at second occurrence on this line.
🤖 Prompt for AI Agents
In packages/react/runtime/src/lynx/lazy-bundle.ts around lines 59 to 67, the
method name "onFullfill" contains a typo and should be corrected to "onFulfill".
Rename the function and all its references from "onFullfill" to "onFulfill" to
fix the typo and resolve pipeline failures.
| const oldCatchError = options[CATCH_ERROR]; | ||
| options[CATCH_ERROR] = (error: any, newVNode: VNode<any>, oldVNode?: VNode<any>, errorInfo?: ErrorInfo) => { | ||
| if (error.then) { | ||
| let component; | ||
| let vnode: VNode<any> | null = newVNode; | ||
| for (; vnode = vnode.__;) { | ||
| if ((component = vnode.__c) && component instanceof LazyBundleResponseListener) { | ||
| component._onLazyBundleResponse(error); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| oldCatchError(error, newVNode, oldVNode, errorInfo); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider the implications of overriding global error handler.
The approach of overriding options[CATCH_ERROR] globally affects all Preact error handling in the application. While this enables catching Promise rejections from lazy loading, it could have unintended side effects on other error handling logic.
Consider documenting this global modification clearly or exploring alternative approaches that don't modify shared global state, such as:
- Using a custom hook that registers/unregisters listeners directly
- Implementing the listener at the lazy loading source rather than through error interception
🤖 Prompt for AI Agents
In packages/react/runtime/src/lynx/lazy-bundle.ts around lines 75 to 89, the
code globally overrides options[CATCH_ERROR], which affects all Preact error
handling and may cause unintended side effects. To fix this, avoid modifying the
global error handler directly; instead, implement error handling locally by
using a custom hook to register and unregister listeners or handle lazy loading
errors at their source. If global override is necessary, add clear documentation
explaining this behavior and its impact on the application.
github-actions
bot
removed
the
stale
1 participant
Summary
Checklist
Summary by CodeRabbit
New Features
Enhancements
Other