feat: add new helper getServer #1441
Conversation
Adi-204
requested review from
asyncapi-bot-eve,
derberg,
jonaslagoni and
magicmatatjahu
as code owners
|
WalkthroughThe pull request introduces a new utility function, Changes
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 (3)
🪧 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]! 👋🏼 |
|
@asyncapi-bot Thank you for the information about the available commands in pull requests. These are helpful for managing PR workflow! I see this PR is about adding a new helper function ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/helpers/src/index.js (1)
4-8: Consider maintaining alphabetical ordering in exports.The new
getServerexport is added at the end, but the export ordering could be more maintainable if kept alphabetical. Consider reordering to:module.exports = { getClientName, + getServer, getServerUrl, - getClientName, - getServer };This is a minor nitpick and doesn't affect functionality.
packages/helpers/test/servers.test.js (1)
75-77: Consider testing null and undefined server names.The test suite covers empty string server names, but consider also testing with
nullandundefinedvalues to ensure complete error handling coverage.it('should throw error when server name is not provided', () => { const serverName = ''; expect(() => { getServer(servers, serverName); }).toThrow(`Server ${serverName} not found in AsyncAPI document.`); }); +it('should throw error when server name is null', () => { + const serverName = null; + + expect(() => { + getServer(servers, serverName); + }).toThrow(`Server ${serverName} not found in AsyncAPI document.`); +}); + +it('should throw error when server name is undefined', () => { + expect(() => { + getServer(servers, undefined); + }).toThrow(`Server undefined not found in AsyncAPI document.`); +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/helpers/src/index.js(1 hunks)packages/helpers/src/servers.js(1 hunks)packages/helpers/test/servers.test.js(2 hunks)packages/templates/clients/js/websocket/template/README.md.js(1 hunks)packages/templates/clients/js/websocket/template/client.js.js(1 hunks)packages/templates/clients/python/websocket/template/client.py.js(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Test NodeJS PR - ubuntu-latest
packages/helpers/src/servers.js
[failure] 44-44:
Missing semicolon
[failure] 39-39:
Missing semicolon
🪛 GitHub Actions: PR testing - if Node project
packages/helpers/src/servers.js
[error] 39-39: ESLint: Missing semicolon (semi)
⏰ 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 (12)
packages/helpers/src/index.js (1)
1-1: Good addition of the new helper function.The import now includes
getServeralongside the existinggetServerUrl, which maintains the module's pattern of importing related functions together.packages/templates/clients/js/websocket/template/README.md.js (2)
2-2: Good use of named imports.The import statement now includes
getServeralongsidegetClientName, maintaining a clean and explicit import style.
5-5: Improved error handling with new helper function.Replacing the direct server access with the new
getServerhelper function enhances error handling for cases where the specified server doesn't exist. This is a good improvement that makes the code more robust.packages/templates/clients/js/websocket/template/client.js.js (2)
2-2: Good addition of the new helper to imports.The import statement has been updated appropriately to include the new
getServerfunction alongside the existing helpers.
8-8: Improved server retrieval with consistent error handling.Replacing direct server access with the
getServerhelper function ensures consistent error handling across the codebase. This is a good practice that improves code reliability.packages/helpers/test/servers.test.js (3)
3-3: Good inclusion of the new helper in test imports.The import statement has been updated to include
getServeralongside the existinggetServerUrlfunction, which is consistent with the implementation file.
44-79: Well-structured test suite with good coverage.The test suite for the new
getServerfunction is comprehensive and follows best practices:
- Tests the happy path (server exists)
- Tests error handling (server doesn't exist)
- Tests edge cases (empty server name)
Each test has clear assertions and good naming that describes the expected behavior. The setup is properly organized with the
beforeAllhook to avoid code duplication.
44-79:✅ Verification successful
Verify server implementation with a validation script.
Let's verify that the implementation of
getServerin the servers.js file properly checks for server existence and throws the expected error.
🏁 Script executed:
#!/bin/bash # Check the implementation of getServer in servers.js echo "Checking for getServer implementation in servers.js" if [ -f "packages/helpers/src/servers.js" ]; then cat packages/helpers/src/servers.js else echo "servers.js not found at expected location" find packages/helpers/src -type f -name "*.js" | xargs grep -l "getServer" fiLength of output: 1748
Verified GetServer Implementation – No Changes Required
The implementation of
getServerinpackages/helpers/src/servers.jscorrectly checks for server existence in the provided servers map and throws the expected error when the server is not found. The reviewed test cases inpackages/helpers/test/servers.test.jsappropriately confirm this behavior.No modifications are needed.
packages/templates/clients/python/websocket/template/client.py.js (2)
2-2: Import updated to include the new helper function.The import statement has been correctly updated to include the new
getServerhelper function alongside the existing helpers.
8-8: Improved server retrieval with error handling.Replacing the direct server access with the
getServerhelper function enhances the code by adding proper error handling when a specified server is not found. This change makes the code more robust and consistent with other templates.packages/helpers/src/servers.js (2)
8-22: Export style modified for consistency.The
getServerUrlfunction has been refactored from a direct module export to a constant declaration, which aligns with the new consolidated export pattern at the end of the file. The function's logic remains unchanged.
46-46: Informative TODO comment for future development.The TODO comment provides valuable context about the organization of helper functions and indicates the direction for future improvements.
packages/helpers/src/servers.js
Outdated
| * @returns {object} The AsyncAPI server object corresponding to the given server name. | ||
| */ | ||
| const getServer = (servers, serverName) => { | ||
| if (serverName && servers.has(serverName)) { |
There was a problem hiding this comment.
@derberg I will add that any more suggested changes?
There was a problem hiding this comment.
no, only this
probably servers = parsedAsyncAPIDocument.servers(); should not be part of beforeAll
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/helpers/src/servers.js (2)
24-39: Well-implemented new helper function with proper error handling.The new
getServerfunction is well-documented with JSDoc comments and implements appropriate error handling when a server isn't found. This abstraction improves code quality across templates by centralizing server retrieval logic.However, you might want to enhance the error handling for edge cases:
const getServer = (servers, serverName) => { - if (serverName && servers.has(serverName)) { + if (!serverName) { + throw new Error('Server name must be provided.'); + } + if (servers.has(serverName)) { return servers.get(serverName); } - throw new Error(`Server ${serverName} not found in AsyncAPI document.`); + throw new Error(`Server "${serverName}" not found in AsyncAPI document. Available servers: ${Array.from(servers.keys()).join(', ')}`); };This would provide more context in error messages and handle empty server names more explicitly.
46-46: Consider turning TODO into a more formal JSDoc module description.This TODO comment provides valuable context about the file's purpose, but it might be better as a formal JSDoc module description at the top of the file.
+/** + * Helpers for working with AsyncAPI server objects. + * + * This file contains utilities for extracting and manipulating data from + * server objects in AsyncAPI documents, including URL construction and server retrieval. + * Future helpers may include variable extraction, security handling, etc. + * @module servers + */ /** * Get server URL from AsyncAPI server object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/helpers/src/servers.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (2)
packages/helpers/src/servers.js (2)
8-8: Code style improvement with consistent function declaration.The function declaration has been updated to use the constant pattern instead of direct export, which aligns better with the module exports pattern used at the end of the file. This improves code organization and maintainability.
41-44: Improved export pattern for better maintainability.Consolidating exports into a single object is a better practice than individual exports, making it easier to see all available functions at a glance.
|
@derberg should I throw different type of errors as suggested by coderabbit or is it ok to throw only 1 type of error. According to me coderabbit suggestion is nice. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/helpers/test/servers.test.js (1)
47-51: Consider initializing servers in thebeforeAllhookSince you're accessing servers in each test case, consider initializing it once in the
beforeAllhook to avoid repetition.beforeAll(async () => { const parseResult = await fromFile(parser, asyncapi_v3_path).parse(); parsedAsyncAPIDocument = parseResult.document; + servers = parsedAsyncAPIDocument.servers(); });packages/helpers/src/servers.js (1)
35-37: Consider checking for undefined/null serversBased on a previous comment from derberg,
serverscan be undefined. Your check handles this case correctly, but consider making the error message more specific about the expected Map type.- if (!servers) { - throw new Error('No server found in AsyncAPI document.'); + if (!servers) { + throw new Error('Server map is undefined or null. A valid Map of AsyncAPI servers must be provided.');🧰 Tools
🪛 GitHub Check: Test NodeJS PR - ubuntu-latest
[failure] 35-35:
Missing space before opening brace
[failure] 35-35:
Expected space(s) after "if"🪛 GitHub Actions: PR testing - if Node project
[error] 35-35: ESLint: Expected space(s) after 'if' (keyword-spacing)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/helpers/src/servers.js(1 hunks)packages/helpers/test/servers.test.js(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Test NodeJS PR - ubuntu-latest
packages/helpers/test/servers.test.js
[failure] 46-46:
'servers' is defined but never used
packages/helpers/src/servers.js
[failure] 38-38:
Missing space before opening brace
[failure] 38-38:
Expected space(s) after "if"
[failure] 35-35:
Missing space before opening brace
[failure] 35-35:
Expected space(s) after "if"
🪛 GitHub Actions: PR testing - if Node project
packages/helpers/src/servers.js
[error] 35-35: ESLint: Expected space(s) after 'if' (keyword-spacing)
⏰ 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 (4)
packages/helpers/test/servers.test.js (2)
44-84: Well-structured test suite for the newgetServerhelperThe test cases thoroughly cover the main scenarios: successful retrieval of an existing server, error handling for non-existent servers, and validation of empty server names. This provides good test coverage for the new functionality.
🧰 Tools
🪛 GitHub Check: Test NodeJS PR - ubuntu-latest
[failure] 46-46:
'servers' is defined but never used
65-73: Good error message testingThe test effectively validates that the error message includes both the server name and a list of available servers, which will be helpful for debugging.
packages/helpers/src/servers.js (2)
41-43: Helpful error message with available serversListing the available servers in the error message is a nice touch that will help developers quickly identify and fix issues.
47-50: Improved module export patternThe consolidated export approach is better than individual exports, making it easier to see all available functions at a glance.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/helpers/src/servers.js (1)
52-52: Consider enhancing the TODO comment.The TODO comment provides good context but seems to be describing the current state rather than future work. Consider rephrasing it to more clearly indicate what specific enhancements are planned.
-//TODO: this separate file for helpers for servers represents approach to keep all helpers in separate files related to extractions of data from specific high level AsyncAPI objects. Here we will have more helpers for example related to variables extraction from servers, security, etc. +//TODO: Add more helper functions for extracting data from server objects, such as variables, security requirements, etc. This follows our approach of organizing helpers by AsyncAPI object type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/helpers/src/servers.js(1 hunks)packages/helpers/test/servers.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/helpers/test/servers.test.js
🧰 Additional context used
🧬 Code Definitions (1)
packages/helpers/src/servers.js (3)
packages/templates/clients/js/websocket/template/client.js.js (2)
server(8-8)serverName(12-12)packages/templates/clients/python/websocket/template/client.py.js (1)
server(8-8)packages/templates/clients/js/websocket/template/README.md.js (1)
server(5-5)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test NodeJS PR - macos-13
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (4)
packages/helpers/src/servers.js (4)
8-22: Well-structured function definition!Converting
getServerUrlfrom a direct export to a local constant is a good practice. This change aligns with the new consolidated export pattern at the end of the file.
24-45: Well-implemented new helper function with comprehensive error handling.The new
getServerfunction is well-documented with detailed JSDoc comments and properly handles three critical error cases:
- Missing servers collection
- Missing server name
- Server not found in the collection
The error message for the third case is particularly helpful as it includes the list of available servers, which will aid debugging when an incorrect server name is provided.
47-50: Improved export pattern for better maintainability.Consolidating exports into a single object is a better practice than individual exports, making it easier to see all available functions at a glance.
34-45:✅ Verification successful
Verify integration with template clients.
The implementation looks solid. Based on the provided code snippets, this function is being used in multiple template clients, which is a good sign of its utility.
🏁 Script executed:
#!/bin/bash # Check how the getServer function is used across the codebase rg -A 2 -B 2 "getServer\(.*servers\(\)" --glob "*.js"Length of output: 1547
Integration Verified: Template Clients Confirmed
The implementation of
getServeris solid and its integration has been verified. The function is consistently used across multiple template clients, including:
packages/templates/clients/js/websocket/template/client.js.jspackages/templates/clients/python/websocket/template/client.py.jspackages/templates/clients/js/websocket/template/README.md.jsNo changes are required based on the current usage.
|
@derberg any more suggestion from your side? thanks : ) |
packages/helpers/src/servers.js
Outdated
| */ | ||
| const getServer = (servers, serverName) => { | ||
| if (!servers) { | ||
| throw new Error('No server found in AsyncAPI document.'); |
There was a problem hiding this comment.
| throw new Error('No server found in AsyncAPI document.'); | |
| throw new Error('Provided AsyncAPI document doesn\'t contain Servers object.'); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/helpers/src/servers.js (2)
24-45: Well-implemented new helper function with proper error handlingThe new
getServerfunction provides excellent error handling for different scenarios:
- When servers collection is missing
- When server name is not provided
- When the specified server doesn't exist in the collection
I particularly like the detailed error message for the missing server case that includes available servers, which will help developers debug issues.
Consider using custom error classes instead of generic Error for better error type distinction. This would allow consumers to catch specific error types:
+ class ServersNotFoundError extends Error { + constructor(message) { + super(message); + this.name = 'ServersNotFoundError'; + } + } + + class ServerNameNotProvidedError extends Error { + constructor(message) { + super(message); + this.name = 'ServerNameNotProvidedError'; + } + } + + class ServerNotFoundError extends Error { + constructor(message) { + super(message); + this.name = 'ServerNotFoundError'; + } + } + const getServer = (servers, serverName) => { if (!servers) { - throw new Error('Provided AsyncAPI document doesn\'t contain Servers object.'); + throw new ServersNotFoundError('Provided AsyncAPI document doesn\'t contain Servers object.'); } if (!serverName) { - throw new Error('Server name must be provided.'); + throw new ServerNameNotProvidedError('Server name must be provided.'); } if (!servers.has(serverName)) { - throw new Error(`Server "${serverName}" not found in AsyncAPI document. Available servers: ${Array.from(servers.keys()).join(', ')}`); + throw new ServerNotFoundError(`Server "${serverName}" not found in AsyncAPI document. Available servers: ${Array.from(servers.keys()).join(', ')}`); } return servers.get(serverName); };
52-52: Consider converting TODO comment to a formal ticketWhile the comment provides good context about the file's purpose and future plans, it would be better to track it in an issue tracking system rather than leaving it as a TODO in the code. This ensures the task won't be forgotten and can be properly prioritized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/helpers/src/servers.js(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/helpers/src/servers.js (3)
packages/templates/clients/js/websocket/template/client.js.js (2)
server(8-8)serverName(12-12)packages/templates/clients/python/websocket/template/client.py.js (1)
server(8-8)packages/templates/clients/js/websocket/template/README.md.js (1)
server(5-5)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (2)
packages/helpers/src/servers.js (2)
8-22: LGTM - Good function implementationThe function has been properly transformed into a named constant. The implementation is clean and well-documented with appropriate JSDoc comments.
47-50: Improved export pattern for better maintainabilityConsolidating exports into a single object is a good practice, making it easier to see all available functions at a glance and maintain the interface. This change aligns with modern JavaScript patterns.
|
@derberg changes done 👍 |
|
|
/rtm |
3 participants
Description
Add new helper function getServer() also written test for it.
Related issue(s)
Fixes #1408
Summary by CodeRabbit
New Features
Tests