chore: refactors readme.md.js of javascript ws client #1512
Conversation
KunalNasa
requested review from
asyncapi-bot-eve,
derberg,
jonaslagoni and
magicmatatjahu
as code owners
|
WalkthroughThis change refactors the WebSocket JavaScript client's README template by extracting the logic for rendering available operations into a new Changes
Sequence Diagram(s)sequenceDiagram
participant README as README Template
participant AO as AvailableOperations
participant OH as OperationHeader
participant ME as MessageExamples
participant Helpers as Helper Functions
README->>AO: Pass operations array
AO->>OH: For each operation, render header
AO->>ME: For each operation, render message example
ME->>Helpers: getOperationMessages, getMessageExamples
ME-->>AO: Render example code snippet
AO-->>README: Rendered operations section
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ Finishing Touches
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:
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 (
|
|
Hello, @coderabbitai[bot]! 👋🏼 |
|
Tip For best results, initiate chat on the files or code changes. @asyncapi-bot Thank you for sharing this helpful information about the available commands! These PR automation commands can definitely streamline the review and merge workflow. I appreciate you explaining what each command does - this is very useful for contributors working with the repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/helpers/src/Operations.js (1)
19-35: Fix inconsistent JSDoc indentation and spacing.The JSDoc for
getMessageExampleshas inconsistent indentation (extra spaces) compared to the first function, and there's an unnecessary empty line after the JSDoc comment.-/** - * Get examples related to the provided message. - * @param {object} message - * @returns {Array} - An array of examples for the provided message. - * @throws {Error} If any of the parameter is missing or invalid. - */ - +/** + * Get examples related to the provided message. + * @param {object} message + * @returns {Array} - An array of examples for the provided message. + * @throws {Error} If any of the parameter is missing or invalid. + */packages/templates/clients/websocket/javascript/components/MessageExamples.js (2)
1-1: Fix trailing comma in import statement.There's an unnecessary trailing comma after
getMessageExamplesthat should be removed.-import { getMessageExamples, } from '@asyncapi/generator-helpers'; +import { getMessageExamples } from '@asyncapi/generator-helpers';
2-2: Consider importing operations helpers from the main package.You're importing
getOperationMessagesdirectly from the operations file while importinggetMessageExamplesfrom the main package. For consistency, consider importing both from the same source if possible.-import { getMessageExamples, } from '@asyncapi/generator-helpers'; -import { getOperationMessages } from '@asyncapi/generator-helpers/src/operations'; +import { getMessageExamples, getOperationMessages } from '@asyncapi/generator-helpers';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/helpers/src/Operations.js(1 hunks)packages/helpers/src/index.js(1 hunks)packages/templates/clients/websocket/javascript/components/AvailableOperations.js(1 hunks)packages/templates/clients/websocket/javascript/components/MessageExamples.js(1 hunks)packages/templates/clients/websocket/javascript/components/OperationHeader.js(1 hunks)packages/templates/clients/websocket/javascript/template/README.md.js(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/templates/clients/websocket/javascript/components/AvailableOperations.js (3)
packages/templates/clients/websocket/javascript/template/README.md.js (1)
operations(10-10)packages/templates/clients/websocket/javascript/components/OperationHeader.js (1)
OperationHeader(3-13)packages/templates/clients/websocket/javascript/components/MessageExamples.js (1)
MessageExamples(5-20)
packages/templates/clients/websocket/javascript/components/OperationHeader.js (1)
packages/templates/clients/websocket/javascript/components/MessageExamples.js (1)
operationId(6-6)
packages/templates/clients/websocket/javascript/components/MessageExamples.js (2)
packages/templates/clients/websocket/javascript/components/OperationHeader.js (1)
operationId(4-4)packages/helpers/src/Operations.js (1)
messages(13-13)
packages/helpers/src/Operations.js (1)
packages/templates/clients/websocket/javascript/components/MessageExamples.js (1)
messages(8-8)
packages/templates/clients/websocket/javascript/template/README.md.js (1)
packages/templates/clients/websocket/javascript/components/AvailableOperations.js (1)
AvailableOperations(5-29)
🪛 Biome (1.9.4)
packages/templates/clients/websocket/javascript/components/AvailableOperations.js
[error] 11-11: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
🔇 Additional comments (10)
packages/helpers/src/Operations.js (2)
9-18: Well-implemented helper function with proper error handling.The
getOperationMessagesfunction is well-designed with appropriate parameter validation and error handling. It correctly throws descriptive error messages when the operation is missing or contains no messages, which will make debugging easier.
26-35: Well-structured function with proper validation.The
getMessageExamplesfunction correctly validates inputs and has appropriate error handling, making it robust for use throughout the application.packages/templates/clients/websocket/javascript/components/OperationHeader.js (1)
3-13: Clean and focused component implementation.This component has a clear, single responsibility of rendering operation headers. It correctly handles the fallback case when an operation summary is not available, and properly extracts channel information.
packages/helpers/src/index.js (1)
1-11: Properly exposed helper functions in the public API.The new helper functions are correctly imported and exported, making them available for use by other components. This update maintains the existing module structure while extending its functionality.
packages/templates/clients/websocket/javascript/components/AvailableOperations.js (1)
5-29: Well-structured component with proper conditional rendering.The component correctly handles both cases: when operations are available and when they are not. The fallback example provides good guidance for users when no operations are defined.
🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
packages/templates/clients/websocket/javascript/components/MessageExamples.js (1)
5-20: Good component extraction for message examples.This component extraction nicely encapsulates the logic for displaying message examples for operations, which improves maintainability. The component will be reused through the
AvailableOperationscomponent seen in the relevant snippets.packages/templates/clients/websocket/javascript/template/README.md.js (4)
3-3: Good modular component usage.Nice job extracting and using the
AvailableOperationscomponent, which makes the template more modular and maintainable.
14-48: Added proper line spacing with newLines prop.Good use of the
newLines={2}prop to ensure proper spacing in the generated markdown.
49-49: Effective component integration for available operations.The
AvailableOperationscomponent is well integrated into the flow of the README template. This modular approach improves code organization and makes future maintenance easier.
50-101: Clean restructuring of template sections.Good job restructuring the template to properly separate the "Available Operations" section from the "Testing the client" section using the new component and text blocks with proper spacing.
packages/templates/clients/websocket/javascript/components/AvailableOperations.js
Outdated
Show resolved
Hide resolved
packages/templates/clients/websocket/javascript/components/MessageExamples.js
Outdated
Show resolved
Hide resolved
| let messageExamples = ''; | ||
| const messages = getOperationMessages(operation); | ||
| const firstMessage = messages[0]; | ||
|
|
||
| const firstExample = getMessageExamples(firstMessage)[0]; | ||
| const payload = firstExample.payload(); | ||
|
|
There was a problem hiding this comment.
Add error handling for missing messages or examples.
The component currently assumes that the operation has at least one message with at least one example. This could lead to runtime errors if an operation doesn't have any messages or if the first message doesn't have examples.
const operationId = operation.id();
let messageExamples = '';
const messages = getOperationMessages(operation);
- const firstMessage = messages[0];
+ const firstMessage = messages && messages.length > 0 ? messages[0] : null;
- const firstExample = getMessageExamples(firstMessage)[0];
- const payload = firstExample.payload();
+ let payload = {};
+ if (firstMessage) {
+ const examples = getMessageExamples(firstMessage);
+ if (examples && examples.length > 0) {
+ const firstExample = examples[0];
+ payload = firstExample.payload();
+ }
+ }📝 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.
| let messageExamples = ''; | |
| const messages = getOperationMessages(operation); | |
| const firstMessage = messages[0]; | |
| const firstExample = getMessageExamples(firstMessage)[0]; | |
| const payload = firstExample.payload(); | |
| const operationId = operation.id(); | |
| let messageExamples = ''; | |
| const messages = getOperationMessages(operation); | |
| const firstMessage = messages && messages.length > 0 ? messages[0] : null; | |
| let payload = {}; | |
| if (firstMessage) { | |
| const examples = getMessageExamples(firstMessage); | |
| if (examples && examples.length > 0) { | |
| const firstExample = examples[0]; | |
| payload = firstExample.payload(); | |
| } | |
| } |
|
@derberg Please review this solution and let me know if any further enhancements are required. |
packages/templates/clients/websocket/javascript/components/AvailableOperations.js
Show resolved
Hide resolved
packages/templates/clients/websocket/javascript/components/OperationHeader.js
Outdated
Show resolved
Hide resolved
| const operationId = operation.id(); | ||
| let messageExamples = ''; | ||
| const messages = getOperationMessages(operation); | ||
| const firstMessage = messages[0]; |
There was a problem hiding this comment.
I think we should support rendering of all examples for all messages, not only first one.
but do not work on it in this PR, make sure to create a followup issue for it once this is merged
There was a problem hiding this comment.
I think you missed this one
Did you mean rendering all examples rather than the first one?
packages/helpers/src/Operations.js
Outdated
| throw new Error('Operation object must be provided.'); | ||
| } | ||
| const messages = operation.messages().all(); | ||
| if (messages === undefined || messages.length === 0) { |
There was a problem hiding this comment.
| if (messages === undefined || messages.length === 0) { | |
| if (!messages) { |
These checks (is undefined, is empty array, and an extra is null check) can be reduced to this line. This is more compact and saves you from having to deal with all falsy values cases.
This also applies to line 31.
There was a problem hiding this comment.
@nightknighto, it will return false if messages would be an empty array ( messages =[] ) . Because empty array is truthy value in JS.
There was a problem hiding this comment.
Made a quick check, an empty array is falsy, so it will work as expected.
There was a problem hiding this comment.
But why it is returning false for !messages
There was a problem hiding this comment.
Looks like we're in the wierd parts now of JavaScript 😁
I guess I've just learned that direct checking on empty arrays may not be a good idea haha. Never mind my suggestion
There was a problem hiding this comment.
I too learned it right now. 😂
There was a problem hiding this comment.
you went to deep into JS 😄
and there is a simpler approach, remember always to consult the Parser API docs
https://github.com/asyncapi/parser-api/blob/master/docs/api.md#messages
you can just do messages().isEmpty() 😉
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/templates/clients/websocket/javascript/components/AvailableOperations.js (1)
5-17: Add handling for empty operations array.The component currently doesn't handle the case when there are no operations to display. Consider adding a conditional to show a message when the operations array is empty.
export function AvailableOperations({ operations }) { return ( <> <Text newLines={2}>### Available Operations</Text> + {operations.length === 0 && ( + <Text>No available operations for this template.</Text> + )} {operations.map((operation) => ( <Text newLines={2}> <OperationHeader operation={operation} /> <MessageExamples operation={operation} /> </Text> ))} </> ); }🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/helpers/src/Operations.js(1 hunks)packages/templates/clients/websocket/javascript/components/AvailableOperations.js(1 hunks)packages/templates/clients/websocket/javascript/components/MessageExamples.js(1 hunks)packages/templates/clients/websocket/javascript/components/OperationHeader.js(1 hunks)packages/templates/clients/websocket/javascript/template/README.md.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/templates/clients/websocket/javascript/components/MessageExamples.js
- packages/templates/clients/websocket/javascript/template/README.md.js
- packages/helpers/src/Operations.js
🧰 Additional context used
🧠 Learnings (1)
packages/templates/clients/websocket/javascript/components/AvailableOperations.js (1)
Learnt from: derberg
PR: asyncapi/generator#1512
File: packages/templates/clients/websocket/javascript/components/AvailableOperations.js:10-15
Timestamp: 2025-04-23T09:18:38.308Z
Learning: In the asyncapi/generator repository, keys aren't required when mapping over operations array in the AvailableOperations component, contrary to the typical React pattern.
🧬 Code Graph Analysis (2)
packages/templates/clients/websocket/javascript/components/OperationHeader.js (1)
packages/templates/clients/websocket/javascript/components/MessageExamples.js (1)
operationId(6-6)
packages/templates/clients/websocket/javascript/components/AvailableOperations.js (3)
packages/templates/clients/websocket/javascript/template/README.md.js (1)
operations(10-10)packages/templates/clients/websocket/javascript/components/OperationHeader.js (1)
OperationHeader(3-12)packages/templates/clients/websocket/javascript/components/MessageExamples.js (1)
MessageExamples(5-22)
🪛 Biome (1.9.4)
packages/templates/clients/websocket/javascript/components/AvailableOperations.js
[error] 10-10: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
🔇 Additional comments (1)
packages/templates/clients/websocket/javascript/components/OperationHeader.js (1)
3-12: LGTM! Clean implementation of the operation header component.The component nicely formats the operation header with ID, summary, and description. The conditional rendering for the description is implemented correctly, and the component follows the suggestions from previous review discussions.
|
Hi @derberg, I have made all the changes suggested by you in comments as well as those discussed in the meeting. Please have a look whenever you get the time and let me know if there is any more refactoring needed or if we are good to go with this solution in the current PR? |
| <Text> | ||
| {`#### \`${operationId}(payload)\` | ||
| ${operation.summary() || ''} | ||
| ${operation.description() ? `\n${operation.description()}` : ''}`} |
There was a problem hiding this comment.
There is hasDescription helper that you can use here in a condition
hasSummary as well is there
https://github.com/asyncapi/parser-api/blob/master/docs/api.md#operation
packages/helpers/src/Operations.js
Outdated
| throw new Error('Operation object must be provided.'); | ||
| } | ||
| const messages = operation.messages().all(); | ||
| if (messages === undefined || messages.length === 0) { |
There was a problem hiding this comment.
you went to deep into JS 😄
and there is a simpler approach, remember always to consult the Parser API docs
https://github.com/asyncapi/parser-api/blob/master/docs/api.md#messages
you can just do messages().isEmpty() 😉
| if (!message) { | ||
| throw new Error('Message object must be provided.'); | ||
| } | ||
| const examples = message.examples(); | ||
| if (examples === undefined || examples.length === 0) { | ||
| throw new Error('No examples found for the provided message.'); | ||
| } | ||
| return examples; |
There was a problem hiding this comment.
same situation like in the other helper
don't throw errors
- might be messages are not there
- might be examples are not there
no errors, we will just not render it in readme
There was a problem hiding this comment.
Ok, I will remove this error throw here and will replace it with a null check.
We had earlier discussed keeping it to encourage users to include examples in messages 😄, but yeah, I get that it’s open to discussion.
I will open a separate issue to discuss about this more. For now, removing this error.
| const operationId = operation.id(); | ||
| let messageExamples = ''; | ||
| const messages = getOperationMessages(operation); | ||
| const firstMessage = messages[0]; |
|
Hi @derberg , I believe I have covered all the suggestions mentioned in the above comments. As for rendering all examples in the readme file, I am planning to create a new PR for that, as you suggested, because that deviates a little bit from the scope of the current PR. If there is any more improvement needed in this PR, then please let me know. 😄 |
|
@derberg I am thinking of opening a follow-up issue for that, as it deviates a little from the scope of our current PR. |
|
definitely makes sense, just next time make it clear so there is no place for assumptions fix failing tests and I can merge |
|
@derberg, Thanks for the feedback. I will make sure to be clearer next time. Also, the failing tests are fixed now. |
actually you did all good, I saw the message and it is clear that I missed it, just read first sentence, my bad, sorry about that |
packages/templates/clients/websocket/javascript/components/MessageExamples.js
Outdated
Show resolved
Hide resolved
|
|
/rtm |
|
@allcontributors please add @KunalNasa for code,doc,bug |
|
I've put up a pull request to add @KunalNasa! 🎉 |
4 participants
This PR refactors the code of readme.md.js file of javascript websocket client.
Introduce three new components for available operations part in readme file.
Introduce two new helper functions related to operation object.
Resolves #1471
Summary by CodeRabbit