refactor: move Connect, OnClose, OnOpen, OnError and OnMessage to @asyncapi/generator-components
#1717
Conversation
…pi/generator-components
Adi-204
requested review from
derberg,
magicmatatjahu and
asyncapi-bot-eve
as code owners
|
WalkthroughAdds centralized, language-aware WebSocket generator components (Connect, OnOpen, OnMessage, OnError, OnClose), exports them from the components package, removes per-template handler implementations, updates templates to import the shared components with language/framework props, and adds snapshot tests for each component. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
What reviewer looks at during PR reviewThe following are ideal points maintainers look for during review. Reviewing these points yourself beforehand can help streamline the review process and reduce time to merge.
|
![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 (13)
packages/components/src/components/OnOpen.js (3)
3-6: Fix JSDoc: this component generates onOpen handlers, not "send" operations.
Update the description to avoid confusion.- * Supported programming languages for WebSocket send operation generation. + * Supported programming languages for WebSocket onOpen handler generation.
11-13: Tighten the mapping type for better editor/tooling support.
Use a precise JSDoc type for the generator function signature.- * @type {Object.<SupportedLanguage, Function>} + * @type {Record<SupportedLanguage, (title: string) => { onOpenMethod: string }>}
23-29: PEP 8 indentation and message consistency.
Use 4 spaces in Python and keep the "server" suffix consistent with JS.- onOpenMethod: `def on_open(self, ws): - print("Connected to ${title} server")` + onOpenMethod: `def on_open(self, ws): + print("Connected to ${title} server")`packages/components/src/components/OnClose.js (2)
3-6: Fix JSDoc: this component generates onClose handlers, not "send" operations.
Align docs with the actual purpose.- * Supported programming languages for WebSocket send operation generation. + * Supported programming languages for WebSocket onClose handler generation.
24-26: PEP 8 indentation and message consistency with other languages.
Use 4 spaces and include the "server" suffix like JS/Dart.- onCloseMethod: `def on_close(self, ws, close_status_code, close_msg): - print("Disconnected from ${title}", close_status_code, close_msg)` + onCloseMethod: `def on_close(self, ws, close_status_code, close_msg): + print("Disconnected from ${title} server", close_status_code, close_msg)`packages/components/test/components/OnClose.test.js (2)
27-27: Fix test name: it says "python" but renders "dart".
Avoid misleading test output.- test('render python OnClose method', () => { + test('render dart OnClose method', () => {
5-36: Optional: reduce duplication with a table-driven test.
Parametrize language/title pairs to avoid repetition.-describe('Testing of OnClose function', () => { - test('render javascript OnClose method', () => { - const result = render( - <OnClose - language='javascript' - title='HoppscotchEchoWebSocketClient' - /> - ); - const actual = result.trim(); - expect(actual).toMatchSnapshot(); - }); - - test('render python OnClose method', () => { - const result = render( - <OnClose - language='python' - title='HoppscotchEchoWebSocketClient' - /> - ); - const actual = result.trim(); - expect(actual).toMatchSnapshot(); - }); - - test('render dart OnClose method', () => { - const result = render( - <OnClose - language='dart' - title='HoppscotchEchoWebSocketClient' - /> - ); - const actual = result.trim(); - expect(actual).toMatchSnapshot(); - }); -}); +describe('Testing of OnClose function', () => { + const cases = [ + ['javascript', 'HoppscotchEchoWebSocketClient'], + ['python', 'HoppscotchEchoWebSocketClient'], + ['dart', 'HoppscotchEchoWebSocketClient'], + ]; + test.each(cases)('render %s OnClose method', (language, title) => { + const result = render(<OnClose language={language} title={title} />); + const actual = result.trim(); + expect(actual).toMatchSnapshot(); + }); +});packages/components/src/index.js (1)
6-11: API surface expansion LGTM; consider keeping exports alphabetized.Alphabetical exports make future diffs cleaner.
-export { MethodGenerator } from './components/MethodGenerator'; -export { Connect } from './components/Connect'; -export { OnOpen } from './components/OnOpen'; -export { OnMessage } from './components/OnMessage'; -export { OnError } from './components/OnError'; -export { OnClose } from './components/OnClose'; +export { Connect } from './components/Connect'; +export { MethodGenerator } from './components/MethodGenerator'; +export { OnClose } from './components/OnClose'; +export { OnError } from './components/OnError'; +export { OnMessage } from './components/OnMessage'; +export { OnOpen } from './components/OnOpen';packages/components/test/components/OnMessage.test.js (1)
1-32: DRY the three near-identical tests via parameterization.Reduces duplication and eases future language additions.
-import { render } from '@asyncapi/generator-react-sdk'; -import { OnMessage } from '../../src/index'; - -describe('Testing of OnMessage function', () => { - test('render javascript OnMessage method', () => { - const result = render( - <OnMessage language='javascript' /> - ); - const actual = result.trim(); - expect(actual).toMatchSnapshot(); - }); - - test('render python OnMessage method', () => { - const result = render( - <OnMessage - language='python' - /> - ); - const actual = result.trim(); - expect(actual).toMatchSnapshot(); - }); - - test('render dart OnMessage method', () => { - const result = render( - <OnMessage - language='dart' - /> - ); - const actual = result.trim(); - expect(actual).toMatchSnapshot(); - }); -}); +import { render } from '@asyncapi/generator-react-sdk'; +import { OnMessage } from '../../src/index'; + +describe('OnMessage renders per language', () => { + const languages = ['javascript', 'python', 'dart']; + it.each(languages)('renders %s OnMessage method', (language) => { + const result = render(<OnMessage language={language} />); + expect(result.trim()).toMatchSnapshot(); + }); +});packages/components/test/components/Connect.test.js (1)
1-37: Add error handling tests for unsupported languages.The test suite covers the happy path for supported languages but doesn't verify the behavior when an unsupported language is provided. Consider adding a test case to ensure the component handles invalid inputs gracefully.
Add a test case for unsupported language handling:
+ test('handle unsupported language gracefully', () => { + const result = render( + <Connect + language='unsupported' + title='TestClient' + /> + ); + const actual = result.trim(); + // Should render empty or a meaningful fallback + expect(actual).toBe(''); + });packages/components/src/components/OnError.js (1)
4-6: Fix JSDoc description inconsistency.The JSDoc description mentions "WebSocket send operation generation" but this component handles the onError event, not send operations.
/** * @typedef {'python' | 'javascript' | 'dart'} SupportedLanguage - * Supported programming languages for WebSocket send operation generation. + * Supported programming languages for WebSocket onError handler generation. */packages/components/src/components/OnMessage.js (1)
4-6: Fix JSDoc description inconsistency.The JSDoc description mentions "WebSocket send operation generation" but this component handles the onMessage event, not send operations.
/** * @typedef {'python' | 'javascript' | 'dart'} SupportedLanguage - * Supported programming languages for WebSocket send operation generation. + * Supported programming languages for WebSocket onMessage handler generation. */packages/components/src/components/Connect.js (1)
8-10: Fix JSDoc description inconsistency.The JSDoc description mentions "WebSocket send operation generation" but this component handles connection establishment, not send operations.
/** * @typedef {'python' | 'javascript' | 'dart'} SupportedLanguage - * Supported programming languages for WebSocket send operation generation. + * Supported programming languages for WebSocket connection method generation. */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (6)
packages/components/test/components/__snapshots__/Connect.test.js.snapis excluded by!**/*.snappackages/components/test/components/__snapshots__/OnClose.test.js.snapis excluded by!**/*.snappackages/components/test/components/__snapshots__/OnError.test.js.snapis excluded by!**/*.snappackages/components/test/components/__snapshots__/OnMessage.test.js.snapis excluded by!**/*.snappackages/components/test/components/__snapshots__/OnOpen.test.js.snapis excluded by!**/*.snappackages/templates/clients/websocket/test/integration-test/__snapshots__/integration.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (17)
packages/components/src/components/Connect.js(1 hunks)packages/components/src/components/OnClose.js(1 hunks)packages/components/src/components/OnError.js(1 hunks)packages/components/src/components/OnMessage.js(1 hunks)packages/components/src/components/OnOpen.js(1 hunks)packages/components/src/index.js(1 hunks)packages/components/test/components/Connect.test.js(1 hunks)packages/components/test/components/OnClose.test.js(1 hunks)packages/components/test/components/OnError.test.js(1 hunks)packages/components/test/components/OnMessage.test.js(1 hunks)packages/components/test/components/OnOpen.test.js(1 hunks)packages/templates/clients/websocket/dart/components/ClientClass.js(2 hunks)packages/templates/clients/websocket/dart/components/Connect.js(0 hunks)packages/templates/clients/websocket/javascript/components/ClientClass.js(2 hunks)packages/templates/clients/websocket/javascript/components/Connect.js(0 hunks)packages/templates/clients/websocket/python/components/ClientClass.js(2 hunks)packages/templates/clients/websocket/python/components/Connect.js(0 hunks)
💤 Files with no reviewable changes (3)
- packages/templates/clients/websocket/python/components/Connect.js
- packages/templates/clients/websocket/javascript/components/Connect.js
- packages/templates/clients/websocket/dart/components/Connect.js
🧰 Additional context used
🧬 Code graph analysis (13)
packages/components/test/components/OnClose.test.js (2)
apps/react-sdk/src/renderer/renderer.ts (1)
render(63-77)packages/components/src/components/OnClose.js (1)
OnClose(45-59)
packages/components/test/components/Connect.test.js (3)
packages/components/src/components/Connect.js (3)
result(112-112)result(115-115)Connect(101-124)packages/components/src/components/OnOpen.js (1)
result(43-43)apps/react-sdk/src/renderer/renderer.ts (1)
render(63-77)
packages/components/test/components/OnError.test.js (2)
apps/react-sdk/src/renderer/renderer.ts (1)
render(63-77)packages/components/src/components/OnError.js (1)
OnError(57-71)
packages/components/test/components/OnMessage.test.js (2)
apps/react-sdk/src/renderer/renderer.ts (1)
render(63-77)packages/components/src/components/OnMessage.js (1)
OnMessage(59-73)
packages/components/src/components/OnOpen.js (2)
packages/templates/clients/websocket/python/components/ClientClass.js (1)
title(14-14)packages/components/src/components/Connect.js (3)
onOpenMethod(102-102)result(112-112)result(115-115)
packages/components/test/components/OnOpen.test.js (2)
packages/components/src/components/OnOpen.js (2)
result(43-43)OnOpen(38-52)apps/react-sdk/src/renderer/renderer.ts (1)
render(63-77)
packages/components/src/components/OnMessage.js (2)
packages/components/src/components/Connect.js (1)
onMessageMethod(103-103)packages/components/src/components/OnError.js (1)
messageResult(62-62)
packages/components/src/components/OnClose.js (2)
packages/templates/clients/websocket/python/components/ClientClass.js (1)
title(14-14)packages/components/src/components/Connect.js (1)
onCloseMethod(105-105)
packages/templates/clients/websocket/dart/components/ClientClass.js (2)
packages/components/src/components/Connect.js (1)
Connect(101-124)packages/templates/clients/websocket/dart/template/client.dart.js (1)
title(9-9)
packages/components/src/components/Connect.js (5)
packages/components/src/components/OnOpen.js (3)
onOpenMethod(39-39)OnOpen(38-52)result(43-43)packages/components/src/components/OnMessage.js (2)
onMessageMethod(60-60)OnMessage(59-73)packages/components/src/components/OnError.js (2)
onErrorMethod(58-58)OnError(57-71)packages/components/src/components/OnClose.js (2)
onCloseMethod(46-46)OnClose(45-59)apps/react-sdk/src/renderer/renderer.ts (1)
render(63-77)
packages/templates/clients/websocket/javascript/components/ClientClass.js (1)
packages/components/src/components/Connect.js (1)
Connect(101-124)
packages/components/src/components/OnError.js (2)
packages/components/src/components/Connect.js (1)
onErrorMethod(104-104)packages/components/src/components/OnMessage.js (1)
messageResult(64-64)
packages/templates/clients/websocket/python/components/ClientClass.js (1)
packages/components/src/components/Connect.js (1)
Connect(101-124)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Acceptance tests for generated templates
🔇 Additional comments (10)
packages/components/src/components/OnOpen.js (1)
16-21: resolve() is scoped correctly; no changes neededresolve() is supplied by the Promise wrapper in Connect.js around the generated connect() method, so the call is safe to retain.
packages/components/src/components/OnClose.js (2)
14-21: LGTM on the JS onClose handler.
Clear and straightforward assignment with a useful log.
30-34:_channelis declared in the Dart WebSocket client template
TheClientFields.jstemplate (packages/templates/clients/websocket/dart/components/ClientFields.js:7) includesWebSocketChannel? _channel;, so usingonDone: () { _channel = null; … }inOnClose.jsis safe.packages/components/test/components/OnError.test.js (1)
4-32: Solid snapshot coverage across languages.
Tests are concise and follow the established render/trim/snapshot pattern.packages/components/test/components/OnOpen.test.js (1)
4-26: LGTM: good coverage for JS and Python.
Consistent pattern with other component tests; snapshots should catch regressions.packages/templates/clients/websocket/dart/components/ClientClass.js (2)
6-6: Centralizing Connect import looks good.Switching to
@asyncapi/generator-componentsaligns with the PR goal and reduces duplication.
17-17: Ensuretitleis passed toClientClassin the Dart template root
I didn’t find any upstream invocations ofClientClassincluding thetitleprop—confirm the root render providestitleso that<Connect>receives it.packages/templates/clients/websocket/python/components/ClientClass.js (2)
8-8: Good move to shared Connect.Importing
Connectfrom components is consistent with the new public API.
26-26: No action required:titleis present and no legacy./Connectimports found.packages/templates/clients/websocket/javascript/components/ClientClass.js (1)
6-6: Shared Connect import is correct.Matches the new centralized component usage.
packages/templates/clients/websocket/javascript/components/ClientClass.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
packages/components/test/components/OnOpen.test.js (2)
18-18: Fix typo in test name: "quarkas" → "quarkus".Improves test output readability.
- test('render java quarkas OnOpen method', () => { + test('render java quarkus OnOpen method', () => {
7-16: Add a regression test for missing/unsupported framework handling.Today, OnOpen throws if language='java' and framework is omitted. After guarding in OnOpen.js (see review there), add a test to ensure it doesn’t throw and renders empty output.
+ test('gracefully handle missing framework for java (no throw, empty)', () => { + const act = () => render(<OnOpen language="java" title="X" />); + expect(act).not.toThrow(); + expect(render(<OnOpen language="java" title="X" />).trim()).toBe(''); + });Also applies to: 18-28
packages/components/test/components/OnClose.test.js (2)
18-18: Fix typo in test name: "quarkas" → "quarkus".Aligns with framework name and other files.
- test('render java quarkas OnClose method', () => { + test('render java quarkus OnClose method', () => {
7-16: Mirror missing-framework regression test for OnClose (java).After adding guards in OnClose.js (see review), ensure java without framework renders empty and doesn’t throw.
+ test('gracefully handle missing framework for java (no throw, empty)', () => { + const act = () => render(<OnClose language="java" title="X" />); + expect(act).not.toThrow(); + expect(render(<OnClose language="java" title="X" />).trim()).toBe(''); + });Also applies to: 18-28
packages/components/src/components/OnOpen.js (2)
8-12: Fix JSDoc: it says “onMessage” for an OnOpen mapping.Update description to “onOpen”.
- * Mapping of supported programming languages to their WebSocket onMessage event handler implementations. + * Mapping of supported programming languages to their WebSocket onOpen event handler implementations.
52-58: Document the framework prop in JSDoc.Aligns with usage across templates.
* @param {Object} props - Component properties. * @param {SupportedLanguage} props.language - The programming language for which to generate onOpen handler code. + * @param {string} [props.framework=''] - Optional framework variant (e.g., 'quarkus' for java). * @param {string} props.title - The title of the WebSocket server.packages/components/src/components/OnClose.js (1)
62-65: JSDoc: document the framework prop.Matches the component API.
* @param {Object} props - Component properties. * @param {SupportedLanguage} props.language - The programming language for which to generate onClose handler code. + * @param {string} [props.framework=''] - Optional framework variant (e.g., 'quarkus' for java). * @param {string} props.title - The title of the WebSocket server.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
packages/components/test/components/__snapshots__/OnClose.test.js.snapis excluded by!**/*.snappackages/components/test/components/__snapshots__/OnOpen.test.js.snapis excluded by!**/*.snappackages/templates/clients/websocket/test/integration-test/__snapshots__/integration.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
packages/components/src/components/OnClose.js(1 hunks)packages/components/src/components/OnOpen.js(1 hunks)packages/components/test/components/Connect.test.js(1 hunks)packages/components/test/components/OnClose.test.js(1 hunks)packages/components/test/components/OnError.test.js(1 hunks)packages/components/test/components/OnMessage.test.js(1 hunks)packages/components/test/components/OnOpen.test.js(1 hunks)packages/templates/clients/websocket/java/quarkus/components/EchoWebSocket.js(2 hunks)packages/templates/clients/websocket/java/quarkus/components/OnClose.js(0 hunks)packages/templates/clients/websocket/java/quarkus/components/OnOpen.js(0 hunks)
💤 Files with no reviewable changes (2)
- packages/templates/clients/websocket/java/quarkus/components/OnOpen.js
- packages/templates/clients/websocket/java/quarkus/components/OnClose.js
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/components/test/components/OnError.test.js
- packages/components/test/components/OnMessage.test.js
- packages/components/test/components/Connect.test.js
🧰 Additional context used
🧬 Code graph analysis (5)
packages/components/test/components/OnOpen.test.js (6)
packages/components/test/components/OnClose.test.js (1)
languages(5-5)packages/components/test/components/OnError.test.js (1)
languages(5-5)packages/components/test/components/OnMessage.test.js (1)
languages(5-5)packages/components/test/components/Connect.test.js (1)
languages(5-5)apps/react-sdk/src/renderer/renderer.ts (1)
render(63-77)packages/components/src/components/OnOpen.js (1)
OnOpen(59-75)
packages/templates/clients/websocket/java/quarkus/components/EchoWebSocket.js (5)
packages/components/src/components/OnOpen.js (1)
OnOpen(59-75)packages/helpers/src/utils.js (2)
title(37-37)title(57-57)packages/templates/clients/websocket/java/quarkus/template/src/main/java/com/asyncapi/client.java.js (1)
title(10-10)packages/templates/clients/websocket/java/quarkus/components/HandleError.js (1)
HandleError(3-13)packages/components/src/components/OnClose.js (1)
OnClose(66-82)
packages/components/src/components/OnOpen.js (3)
packages/templates/clients/websocket/python/components/ClientClass.js (1)
title(14-14)packages/components/src/components/Connect.js (1)
onOpenMethod(102-102)packages/components/src/components/OnClose.js (2)
config(50-50)indent(68-68)
packages/components/src/components/OnClose.js (3)
packages/templates/clients/websocket/python/components/ClientClass.js (1)
title(14-14)packages/components/src/components/Connect.js (1)
onCloseMethod(105-105)packages/components/src/components/OnOpen.js (2)
config(43-43)indent(61-61)
packages/components/test/components/OnClose.test.js (2)
apps/react-sdk/src/renderer/renderer.ts (1)
render(63-77)packages/components/src/components/OnClose.js (1)
OnClose(66-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Acceptance tests for generated templates
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - macos-13
- GitHub Check: Test NodeJS PR - ubuntu-latest
🔇 Additional comments (3)
packages/templates/clients/websocket/java/quarkus/components/EchoWebSocket.js (2)
4-4: Good move to shared components; props look correct for Java/Quarkus.Named imports from the shared package and passing framework/title are consistent with the new API.
Also applies to: 23-27, 30-34
4-4: Resolved: Public exports for OnOpen/OnClose are wired
OnOpen and OnClose are exported from packages/components/src/index.js (lines 8, 11) and no local imports remain.packages/components/src/components/OnOpen.js (1)
14-21: No undefinedresolve()issue:onOpenMethodis always injected into thenew Promise((resolve, reject) => { … })inConnect.js, soresolve()is defined and no further gating is needed.
|
|
@derberg the PR is ready for review💪 |
Adi-204
changed the title
Connect, OnClose, OnOpen, OnError and OnMessage to @asyncapi/generator-componentsConnect, OnClose, OnOpen, OnError and OnMessage to @asyncapi/generator-components
|
/rtm |
Description
Move
Connect,OnClose,OnOpen,OnErrorandOnMessageto@asyncapi/generator-components. Need to refactor all in single PR.Related issue(s)
Fixes #1716
Summary by CodeRabbit
New Features
Refactor
Tests