-
Notifications
You must be signed in to change notification settings - Fork 111
fix(testing): GlobalEventEmitter.emit should receive array #1479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 93db619 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 |
📝 WalkthroughWalkthroughThis change introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Suggested labels
Suggested reviewers
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/testing-library/testing-environment/src/lynx/GlobalEventEmitter.ts (1)
2-6: Tighten types and simplify emit logicPrefer a typed listener signature over
Function, and remove the redundant truthy check onargs. This makes the API safer and simpler without changing behavior.Apply this diff:
+type Listener = (...args: unknown[]) => void; + export class GlobalEventEmitter { - listeners: Record<string, Function[]> = {}; - addListener(eventName: string, listener: Function): void { + listeners: Record<string, Listener[]> = {}; + addListener(eventName: string, listener: Listener): void { this.listeners[eventName] ??= []; this.listeners[eventName].push(listener); } - removeListener(eventName: string, listener: Function): void { + removeListener(eventName: string, listener: Listener): void { if (!this.listeners[eventName]) { return; } this.listeners[eventName] = this.listeners[eventName].filter((l) => l !== listener ); } - emit(eventName: string, args: any[]): void { + emit(eventName: string, args: unknown[]): void { if (!this.listeners[eventName]) { return; } - this.listeners[eventName].forEach((listener) => - args ? listener(...args) : listener() - ); + this.listeners[eventName].forEach((listener) => listener(...args)); } @@ - trigger(eventName: string, params: string | Record<any, any>): void { + trigger(eventName: string, params: unknown): void { this.emit(eventName, [params]); } - toggle(eventName: string, ...data: unknown[]): void { + toggle(eventName: string, ...data: unknown[]): void { this.emit(eventName, data); }Also applies to: 7-14, 15-21, 33-38
packages/react/testing-library/src/__tests__/lynx.test.jsx (1)
9-14: Add coverage for trigger/toggle (optional)To lock in the convenience APIs’ behavior, consider a small follow-up test.
Example:
it('trigger and toggle should forward params correctly', () => { const cb = vi.fn(); const ge = lynx.getJSModule('GlobalEventEmitter'); ge.addListener('evt', cb); ge.trigger('evt', { a: 1 }); ge.toggle('evt', 1, 2, 3); expect(cb.mock.calls).toEqual([[{ a: 1 }], [1, 2, 3]]); });packages/testing-library/testing-environment/src/index.ts (1)
339-372: Optionally narrow types for getJSModuleIf you want stronger type help at callsites, narrow the accepted module name and return type.
- getJSModule: (moduleName) => { + getJSModule: (moduleName: 'GlobalEventEmitter'): GlobalEventEmitter => { if (moduleName === 'GlobalEventEmitter') { return globalEventEmitter; } else { throw new Error(`getJSModule(${moduleName}) not implemented`); } },.changeset/khaki-beans-enter.md (1)
5-5: Fix typos and phrasing in changesetMinor spelling/grammar polish.
-Fix `GlobalEventEmitter` type definition, the `emit(eventName: string, data: unknown)` function should recevie an array typed `data` and pass as param list of listeners. +Fix `GlobalEventEmitter` type definition: the `emit(eventName: string, data: unknown)` function should receive an array-typed `data` and pass it as the parameter list to listeners.packages/react/runtime/__test__/utils/jsModule.ts (2)
5-16: Unify listener typing and simplify emitMirror the type tightening from the main implementation and simplify the emit body.
+type Listener = (...args: unknown[]) => void; + class GlobalEventEmitter { - listeners: Record<string, Function[]> = {}; - addListener(eventName: string, listener: Function): void { + listeners: Record<string, Listener[]> = {}; + addListener(eventName: string, listener: Listener): void { this.listeners[eventName] ??= []; this.listeners[eventName].push(listener); } - removeListener(eventName: string, listener: Function): void { + removeListener(eventName: string, listener: Listener): void { if (!this.listeners[eventName]) { return; } this.listeners[eventName] = this.listeners[eventName].filter((l) => l !== listener); } - emit(eventName: string, args: any[]): void { + emit(eventName: string, args: unknown[]): void { if (!this.listeners[eventName]) { return; } - this.listeners[eventName].forEach((listener) => args ? listener(...args) : listener()); + this.listeners[eventName].forEach((listener) => listener(...args)); } @@ - removeAllListeners(eventName?: string): void { + removeAllListeners(eventName?: string): void { if (eventName) { delete this.listeners[eventName]; } else { this.clear(); } } - trigger(eventName: string, params: string | Record<any, any>): void { + trigger(eventName: string, params: unknown): void { this.emit(eventName, [params]); } toggle(eventName: string, ...data: unknown[]): void { this.emit(eventName, data); }Also applies to: 17-22, 26-38
5-16: Avoid code duplication if feasible (optional)If package boundaries allow, consider importing the shared
GlobalEventEmitterto avoid divergence between test utils and testing-environment.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/khaki-beans-enter.md(1 hunks)packages/react/runtime/__test__/utils/jsModule.ts(1 hunks)packages/react/testing-library/src/__tests__/lynx.test.jsx(1 hunks)packages/testing-library/testing-environment/src/index.ts(2 hunks)packages/testing-library/testing-environment/src/lynx/GlobalEventEmitter.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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.
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#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.
📚 Learning: 2025-08-06T13:28:57.139Z
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/react/testing-library/src/__tests__/lynx.test.jsxpackages/testing-library/testing-environment/src/index.ts
📚 Learning: 2025-08-06T13:28:57.139Z
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/react/testing-library/src/__tests__/lynx.test.jsxpackages/testing-library/testing-environment/src/index.ts
📚 Learning: 2025-08-07T04:00:59.627Z
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:
packages/react/testing-library/src/__tests__/lynx.test.jsx.changeset/khaki-beans-enter.md
📚 Learning: 2025-07-16T06:26:22.230Z
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/react/testing-library/src/__tests__/lynx.test.jsx
📚 Learning: 2025-07-22T09:23:07.797Z
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/khaki-beans-enter.md
🧬 Code Graph Analysis (1)
packages/testing-library/testing-environment/src/index.ts (1)
packages/testing-library/testing-environment/src/lynx/GlobalEventEmitter.ts (1)
GlobalEventEmitter(1-39)
🪛 LanguageTool
.changeset/khaki-beans-enter.md
[grammar] ~5-~5: Ensure spelling is correct
Context: ...string, data: unknown)function should recevie an array typeddata` and pass as param...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (4)
packages/testing-library/testing-environment/src/lynx/GlobalEventEmitter.ts (1)
15-21: Array-based emit is correctly implementedSpreading the array into listener invocation is aligned with the PR objective and looks good.
packages/react/testing-library/src/__tests__/lynx.test.jsx (1)
9-14: Test aligned with new emit signatureWrapping the payload in an array matches the new API and the snapshot expectation. LGTM.
packages/testing-library/testing-environment/src/index.ts (2)
13-13: Integration of GlobalEventEmitter looks correctImporting and exposing a single instance via
lynx.getJSModule('GlobalEventEmitter')meets the new API. No functional issues spotted.Also applies to: 339-372
339-372: All GlobalEventEmitter.emit calls use an array or no payloadI’ve scanned every
getJSModule('GlobalEventEmitter').emitusage and found that all calls either omit the second argument or pass an array-wrapped payload—no bare object literals remain. No breaking changes required.
CodSpeed Performance ReportMerging #1479 will not alter performanceComparing Summary
|
Web Explorer#3941 Bundle Size — 343.8KiB (0%).93db619(current) vs ea325d6 main#3930(baseline) Bundle metrics
Bundle size by type
|
| Current #3941 |
Baseline #3930 |
|
|---|---|---|
229.01KiB |
229.01KiB |
|
82.95KiB |
82.95KiB |
|
31.84KiB |
31.84KiB |
Bundle analysis report Branch upupming:fix/GlobalEventEmitter Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#3945 Bundle Size — 235.26KiB (0%).93db619(current) vs ea325d6 main#3934(baseline) Bundle metrics
|
| Current #3945 |
Baseline #3934 |
|
|---|---|---|
0B |
0B |
|
0B |
0B |
|
0% |
0% |
|
0 |
0 |
|
4 |
4 |
|
159 |
159 |
|
64 |
64 |
|
45.81% |
45.81% |
|
2 |
2 |
|
0 |
0 |
Bundle size by type no changes
| Current #3945 |
Baseline #3934 |
|
|---|---|---|
145.76KiB |
145.76KiB |
|
89.5KiB |
89.5KiB |
Bundle analysis report Branch upupming:fix/GlobalEventEmitter Project dashboard
Generated by RelativeCI Documentation Report issue
…ly#1479) <!-- Thank you for submitting a pull request! We appreciate the time and effort you have invested in making these changes. Please ensure that you provide enough information to allow others to review your pull request. Upon submission, your pull request will be automatically assigned with reviewers. If you want to learn more about contributing to this project, please visit: https://github.com/lynx-family/lynx-stack/blob/main/CONTRIBUTING.md. --> <!-- The AI summary below will be auto-generated - feel free to replace it with your own. --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **New Features** * Introduced a global event management system with a new event emitter supporting adding, removing, and clearing listeners. * Added convenience methods for emitting events with flexible argument handling. * **Bug Fixes** * Corrected event emission to properly handle multiple arguments when notifying listeners. * **Tests** * Updated tests to align with the new event emission method. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!--- Check and mark with an "x" --> - [x] Tests updated (or not required). - [ ] Documentation updated (or not required). - [x] Changeset added, and when a BREAKING CHANGE occurs, it needs to be clearly marked (or not required).
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>
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Checklist