refactor: move RegisterMessageHandler and MethodGenerator component to @asyncapi/generator-components
#1663
Conversation
Adi-204
requested review from
asyncapi-bot-eve,
derberg,
jonaslagoni and
magicmatatjahu
as code owners
|
WalkthroughA new reusable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Possibly related PRs
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
|
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: 6
🧹 Nitpick comments (3)
packages/components/test/components/RegisterMessageHandler.test.js (1)
1-83: Solid snapshot coverage across languages (LGTM) + add a negative testCoverage looks good: default rendering, custom names, params, and pre/post code across JS/Python/Dart. Two improvements:
- Add a negative test asserting a clear error for unsupported language (e.g., language='go') to lock in failure mode if configs are missing.
- Optional: Prefer inline snapshots for faster review and to keep tests self-contained.
If you want, I can propose a negative test snippet for an unsupported language.
packages/components/src/components/RegisterMessageHandler.js (2)
24-27: Python block indentation: keep inner suites consistentThe if/else suites use different inner indentation widths (2 vs 4). Make them consistent to avoid style regressions in generated code.
if callable(handler): self.message_handlers.append(handler) else: - print("Message handler must be callable") + print("Message handler must be callable")
46-53: JSDoc defaults: use bracket syntax for optional paramsUse standard JSDoc optional/default notation to improve tooling support.
- * @param {Object} props - Component props. - * @param {Language} props.language - Programming language used for method formatting. - * @param {string} props.methodName='registerMessageHandler' - Name of the method to generate. - * @param {string[]} props.methodParams=[] - List of parameters for the method. - * @param {string} props.preExecutionCode - Code to insert before the main function logic. - * @param {string} props.postExecutionCode - Code to insert after the main function logic. + * @param {Object} props - Component props. + * @param {Language} props.language - Programming language used for method formatting. + * @param {string} [props.methodName='registerMessageHandler'] - Name of the method to generate. + * @param {string[]} [props.methodParams=[]] - List of parameters for the method. + * @param {string} [props.preExecutionCode] - Code to insert before the main function logic. + * @param {string} [props.postExecutionCode] - Code to insert after the main function logic.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/components/test/components/__snapshots__/RegisterMessageHandler.test.js.snapis excluded by!**/*.snappackages/templates/clients/websocket/test/integration-test/__snapshots__/integration.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
packages/components/src/components/RegisterMessageHandler.js(1 hunks)packages/components/src/index.js(1 hunks)packages/components/test/components/RegisterMessageHandler.test.js(1 hunks)packages/templates/clients/websocket/dart/components/ClientClass.js(2 hunks)packages/templates/clients/websocket/dart/components/RegisterMessageHandler.js(0 hunks)packages/templates/clients/websocket/javascript/components/ClientClass.js(2 hunks)packages/templates/clients/websocket/javascript/components/RegisterMessageHandler.js(0 hunks)packages/templates/clients/websocket/python/components/ClientClass.js(2 hunks)packages/templates/clients/websocket/python/components/RegisterMessageHandler.js(0 hunks)
💤 Files with no reviewable changes (3)
- packages/templates/clients/websocket/dart/components/RegisterMessageHandler.js
- packages/templates/clients/websocket/javascript/components/RegisterMessageHandler.js
- packages/templates/clients/websocket/python/components/RegisterMessageHandler.js
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/templates/clients/websocket/dart/components/ClientClass.js (1)
packages/components/src/components/RegisterMessageHandler.js (1)
RegisterMessageHandler(54-91)
packages/templates/clients/websocket/python/components/ClientClass.js (1)
packages/components/src/components/RegisterMessageHandler.js (1)
RegisterMessageHandler(54-91)
packages/templates/clients/websocket/javascript/components/ClientClass.js (1)
packages/components/src/components/RegisterMessageHandler.js (1)
RegisterMessageHandler(54-91)
⏰ 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: Acceptance tests for generated templates
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - macos-13
- GitHub Check: Test NodeJS PR - ubuntu-latest
🔇 Additional comments (6)
packages/components/src/index.js (1)
4-5: Exports Verified: RegisterMessageHandler is properly exposedI’ve confirmed that:
packages/components/src/components/RegisterMessageHandler.jsexists.packages/components/src/index.jsre-exports it.packages/components/package.json’smainfield points tolib/index.js, so downstream consumers will resolve it correctly.- There are no shipped TypeScript definitions in this package, so no additional type exports are needed.
packages/templates/clients/websocket/javascript/components/ClientClass.js (1)
18-21: Props are correct for JS (LGTM)language="javascript" with methodParams={['handler']} aligns with the shared component’s defaults and expected JS signature.
packages/templates/clients/websocket/python/components/ClientClass.js (2)
28-33: Python method signature and docstring positioning look correct (LGTM)
- methodName='register_message_handler' and methodParams={['self', 'handler']} are idiomatic for Python.
- Using preExecutionCode with a triple-quoted string ensures it becomes the function’s docstring when it’s the first body statement.
40-44: Consistent CloseConnection usage (LGTM)CloseConnection configured with language="python" and a docstring is consistent with this refactor.
packages/templates/clients/websocket/dart/components/ClientClass.js (2)
19-22: Dart method signature is well-typed (LGTM)Using methodParams={['void Function(String) handler']} yields a clear, typed Dart signature and aligns with default methodName for Dart.
26-26: CloseConnection consistency (LGTM)language="dart" set explicitly — consistent with the shared-component approach.
packages/templates/clients/websocket/javascript/components/ClientClass.js
Show resolved
Hide resolved
|
Adi-204
changed the title
RegisterMessageHandler component to @asyncapi/generator-componentsRegisterMessageHandler and MethodGenerator component to @asyncapi/generator-components
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
packages/components/src/components/RegisterMessageHandler.js (1)
44-49: Guard against unsupported languages to avoid runtime crash
websocketMessageRegisterConfig[language]may beundefined. Add a safeguard before destructuring.export function RegisterMessageHandler({ language, methodName = 'registerMessageHandler', methodParams = [], preExecutionCode = '', postExecutionCode = '' }) { - const { - methodDocs = '', - methodLogic = '' - } = websocketMessageRegisterConfig[language]; + const wsCfg = websocketMessageRegisterConfig[language]; + if (!wsCfg) { + throw new Error(`Unsupported language: ${language}`); + } + const { + methodDocs = '', + methodLogic = '', + } = wsCfg;packages/components/src/components/MethodGenerator.js (2)
12-16: Prefer 4-space Python indentation and control openingTag spacing via config
- Python typically uses 4 spaces; current
indentSize: 2diverges from common style.- Let config carry spacing needs (e.g.,
' {'for JS/Dart,':'for Python) instead of hardcoding builder spaces.This reduces style issues and aligns with language norms.
-const defaultMethodConfig = { - python: { returnType: 'def', openingTag: ':', indentSize: 2 }, - javascript: { openingTag: '{', closingTag: '}', indentSize: 2 }, - dart: { returnType: 'void', openingTag: '{', closingTag: '}', indentSize: 2 } -}; +const defaultMethodConfig = { + python: { returnType: 'def', openingTag: ':', indentSize: 4 }, + // Opening brace includes leading space to avoid manual spacing in the builder + javascript: { openingTag: ' {', closingTag: '}', indentSize: 2 }, + dart: { returnType: 'void', openingTag: ' {', closingTag: '}', indentSize: 2 }, +};
69-72: Fix signature construction: remove unconditional leading blank line and stray spacesCurrent template:
- Adds a blank line when
methodDocsis empty.- Inserts a leading space when
returnTypeis empty (JS/Dart).- Forces a space before Python colon:
def foo() :.Build signature and lines conditionally.
- const methodCode = `${methodDocs} -${returnType} ${methodName}(${params}) ${openingTag} -${indentedLogic} -${closingTag}`; + const signatureParts = []; + if (returnType) signatureParts.push(returnType); + signatureParts.push(`${methodName}(${params})`); + // openingTag includes any required spacing (e.g., ' {' for JS/Dart, ':' for Python) + const signature = `${signatureParts.join(' ')}${openingTag}`; + + const methodLines = []; + if (methodDocs) methodLines.push(methodDocs); + methodLines.push(signature); + if (indentedLogic) methodLines.push(indentedLogic); + if (closingTag) methodLines.push(closingTag); + const methodCode = methodLines.join('\n');
🧹 Nitpick comments (4)
packages/components/test/components/MethodGenerator.test.js (1)
91-99: Empty method body yields a blank line — decide desired style and assert itWith no
methodLogic, current output contains an empty line between signature and closing brace. If you prefer compactfunction() {}/def foo():without a blank line, updateMethodGeneratorto omit the body line when empty and adjust snapshots. Otherwise, add a test asserting the intentional blank line to lock the style in.packages/components/src/components/CloseConnection.js (1)
51-61: PassnewLinesexplicitly for consistent spacing around generated methodsOther components use
newLines={2}to separate methods. If consistent spacing is desired, passnewLineshere too (or centralize the default).- <MethodGenerator + <MethodGenerator language={language} methodName={methodName} methodParams = {methodParams} methodDocs = {methodDocs} methodLogic = {methodLogic} preExecutionCode = {preExecutionCode} postExecutionCode = {postExecutionCode} indent = {2} + newLines = {2} />packages/components/src/components/RegisterMessageHandler.js (1)
50-61: Ensurehandleris part ofmethodParamsin caller usageThe logic references
handler; if callers forget to pass it inmethodParams, generated code will be incorrect. Consider validatingmethodParamsincludes the appropriate argument (or document prominently).packages/components/src/components/MethodGenerator.js (1)
63-68: Avoid indenting empty lines to reduce trailing whitespace and noiseCurrent mapping adds empty strings (lines with only EOL). After the signature fix above (which already omits empty indented body), behavior is improved. If you keep current approach, consider filtering empty lines before join.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/components/test/components/__snapshots__/MethodGenerator.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
packages/components/src/components/CloseConnection.js(2 hunks)packages/components/src/components/MethodGenerator.js(1 hunks)packages/components/src/components/RegisterMessageHandler.js(1 hunks)packages/components/src/index.js(1 hunks)packages/components/test/components/MethodGenerator.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/src/index.js
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/components/src/components/CloseConnection.js (1)
packages/components/src/components/MethodGenerator.js (1)
MethodGenerator(33-79)
packages/components/test/components/MethodGenerator.test.js (2)
apps/react-sdk/src/renderer/renderer.ts (1)
render(63-77)packages/components/src/components/MethodGenerator.js (1)
MethodGenerator(33-79)
packages/components/src/components/RegisterMessageHandler.js (1)
packages/components/src/components/MethodGenerator.js (1)
MethodGenerator(33-79)
⏰ 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 20
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Acceptance tests for generated templates
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - ubuntu-latest
- GitHub Check: Test NodeJS PR - macos-13
|
@derberg I had to add 2 components in one PR because sonar was complaining about code duplication. And now I think it is much more easier to add new components of websocket template and minimal changes to be done. |
derberg
left a comment
derberg
left a comment
There was a problem hiding this comment.
Interesting approach, let's give it a try, nice job!
|
/rtm |
3 participants
Description
Add new component
RegisterMessageHandlerand refactor existing codebase.Related issue(s)
Fixes #1662
Summary by CodeRabbit
New Features
Refactor
Tests