chore: adopted new file structure and update README and example.js with postmanClient and hoppscotchClient. #1476
Conversation
Light13008
requested review from
Florence-Njeri,
derberg,
asyncapi-bot-eve,
magicmatatjahu and
jonaslagoni
as code owners
|
WalkthroughThis pull request updates several file paths and constants for the WebSocket client example and integration tests, along with an enhancement in the generator's npm path resolution. The README and example files now reference new client library directories, the integration tests introduce distinct result paths for different clients, and the generator code improves error handling when resolving the npm module. Changes
Sequence Diagram(s)sequenceDiagram
participant G as Generator
participant R as requireg
G->>R: resolve('npm')
alt Successful resolution
R-->>G: npmResolvedPath
G->>G: Set npmPath using npmResolvedPath (replace 'index.js')
else Failure
R-->>G: Error thrown
G->>G: Set npmPath to empty string
end
Assessment against linked issues
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 LanguageToolpackages/templates/clients/websocket/javascript/README.md[uncategorized] ~9-~9: Did you mean: “By default,”? (BY_DEFAULT_COMMA) [uncategorized] ~9-~9: You might be missing the article “the” here. (AI_EN_LECTOR_MISSING_DETERMINER_THE) [uncategorized] ~9-~9: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE) 🪧 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 (
|
![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
🔭 Outside diff range comments (7)
packages/templates/clients/websocket/javascript/test/integration.test.js (7)
32-33: 🛠️ Refactor suggestionUpdate test to use the new directory structure.
The test still reads the client file from the old location. Consider updating it to use the new
testResultPathPostmanconstant.- const client = await readFile(path.join(testResultPath, testOutputFile), 'utf8'); + const client = await readFile(path.join(testResultPathPostman, testOutputFile), 'utf8'); expect(client).toMatchSnapshot();
49-50: 🛠️ Refactor suggestionUpdate test to use the new directory structure.
Similar to the previous test, this one also reads the client file from the old location. Consider updating it to use the new
testResultPathHoppscotchconstant.- const client = await readFile(path.join(testResultPath, testOutputFile), 'utf8'); + const client = await readFile(path.join(testResultPathHoppscotch, testOutputFile), 'utf8'); expect(client).toMatchSnapshot();
65-69: 🛠️ Refactor suggestionUpdate test to use the new directory structure.
This test should also be updated to use the new directory structure for consistency.
- const clientOutputFile = path.join(testResultPath, defaultOutputFile); + const clientOutputFile = path.join(testResultPathClient, defaultOutputFile); const checkClientOutputFileExists = await stat(clientOutputFile); expect(checkClientOutputFileExists.isFile()).toBeTruthy();
22-28: 🛠️ Refactor suggestionUpdate Generator initialization to use the new path constants.
For the first test case, update the Generator initialization to use the new
testResultPathPostmanconstant.- const generator = new Generator(template, testResultPath, { + const generator = new Generator(template, testResultPathPostman, { forceWrite: true, templateParams: { server: 'echoServer', clientFileName: testOutputFile } });
39-45: 🛠️ Refactor suggestionUpdate Generator initialization to use the new path constants.
For the second test case, update the Generator initialization to use the new
testResultPathHoppscotchconstant.- const generator = new Generator(template, testResultPath, { + const generator = new Generator(template, testResultPathHoppscotch, { forceWrite: true, templateParams: { server: 'echoServer', clientFileName: testOutputFile } });
56-61: 🛠️ Refactor suggestionUpdate Generator initialization to use the new path constants.
For the third test case, update the Generator initialization to use the new
testResultPathClientconstant.- const generator = new Generator(template, testResultPath, { + const generator = new Generator(template, testResultPathClient, { forceWrite: true, templateParams: { server: 'echoServer', } });
75-80: 🛠️ Refactor suggestionUpdate Generator initialization to use the new path constants.
For consistency, update the Generator initialization in the fourth test case as well.
- const generator = new Generator(template, testResultPath, { + const generator = new Generator(template, testResultPathHoppscotch, { forceWrite: true, templateParams: { clientFileName: testOutputFile } });
🧹 Nitpick comments (11)
packages/templates/clients/websocket/javascript/README.md (1)
9-9: Improve readability with proper punctuation and articles.The added explanatory text would benefit from improved grammar to enhance clarity for users.
-> By default this is testing against Postman echo service. You can modify `packages/templates/clients/websocket/javascript/example.js` and change first line to `const WSClient = require('./test/temp/snapshotTestResult/hoppscotchClient/client-hoppscotch.js');` and run `node example.js` again. You will see example still works but now it is using a different API. This is possible since both AsyncAPI documents define the same name of operation for sending messages: `sendEchoMessage` so each client generated has the same API. +> By default, this is testing against Postman echo service. You can modify `packages/templates/clients/websocket/javascript/example.js` and change the first line to `const WSClient = require('./test/temp/snapshotTestResult/hoppscotchClient/client-hoppscotch.js');` and run `node example.js` again. You will see the example still works, but now it is using a different API. This is possible since both AsyncAPI documents define the same name of operation for sending messages: `sendEchoMessage` so each client generated has the same API.🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: Did you mean: “By default,”?
Context: ...rated by the test:node example.js> By default this is testing against Postman echo se...(BY_DEFAULT_COMMA)
[uncategorized] ~9-~9: You might be missing the article “the” here.
Context: ...ocket/javascript/example.jsand change first line toconst WSClient = require('./te...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~9-~9: Possible missing article found.
Context: ...nnode example.jsagain. You will see example still works but now it is using a diffe...(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~9-~9: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... again. You will see example still works but now it is using a different API. This i...(COMMA_COMPOUND_SENTENCE)
packages/templates/clients/websocket/javascript/test/temp/snapshotTestResult/postmanClient/client-postman.js (4)
12-12: Consider simplifying the class name to avoid redundancy.The class name
PostmanEchoWebSocketClientClientcontains a redundant "Client" suffix.-class PostmanEchoWebSocketClientClient { +class PostmanEchoWebSocketClient {Remember to also update the export statement at the end of the file:
-module.exports = PostmanEchoWebSocketClientClient; +module.exports = PostmanEchoWebSocketClient;
61-68: Fix method documentation comment.The comment above this method incorrectly states it's for registering error handlers, but it's actually for message handlers.
- // Method to register custom error handlers + // Method to register custom message handlers registerMessageHandler(handler) { if (typeof handler === 'function') { this.messageHandlers.push(handler); } else { console.warn('Message handler must be a function'); } }
25-29: Add connection state tracking.The
connectmethod doesn't track the connection state, which could lead to multiple connection attempts or attempts to use a disconnected socket.connect() { return new Promise((resolve, reject) => { + if (this.websocket && this.websocket.readyState === WebSocket.OPEN) { + console.log('Already connected to Postman Echo WebSocket Client server'); + resolve(); + return; + } + this.websocket = new WebSocket(this.url); // On successful connection this.websocket.onopen = () => { console.log('Connected to Postman Echo WebSocket Client server'); resolve(); };
90-96: Return a Promise from the close method for consistency.For consistency with the
connectmethod, consider making theclosemethod return a Promise as well.// Method to close the WebSocket connection close() { - if (this.websocket) { - this.websocket.close(); - console.log('WebSocket connection closed.'); - } + return new Promise((resolve) => { + if (!this.websocket) { + console.log('No WebSocket connection to close.'); + resolve(); + return; + } + + this.websocket.onclose = () => { + console.log('WebSocket connection closed.'); + this.websocket = null; + resolve(); + }; + + this.websocket.close(); + }); }packages/templates/clients/websocket/javascript/test/temp/snapshotTestResult/postmanClient/README.md (1)
37-44: Consider adding response type information.The documentation for
sendEchoMessagecould be enhanced by specifying what type of response to expect after sending a message. This would help users understand the complete request-response cycle.packages/templates/clients/websocket/javascript/test/temp/snapshotTestResult/hoppscotchClient/README.md (2)
5-5: Fix typo in overview section.There's a typo in "Undestand" which should be "Understand".
-Undestand how to use Hoppscotch Echo WebSocket as a client. Hoppscotch Echo WebSocket server echoes back any messages sent to it. You can use this to test WebSocket connections and message flows. +Understand how to use Hoppscotch Echo WebSocket as a client. Hoppscotch Echo WebSocket server echoes back any messages sent to it. You can use this to test WebSocket connections and message flows.
39-39: Add missing article in the sentence.Missing "the" before "server" makes the sentence grammatically incorrect.
-Receive the timestamp message sent from server every second. +Receive the timestamp message sent from the server every second.🧰 Tools
🪛 LanguageTool
[uncategorized] ~39-~39: You might be missing the article “the” here.
Context: ...Receive the timestamp message sent from server every second. Example: ```javascr...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
packages/templates/clients/websocket/javascript/test/temp/snapshotTestResult/hoppscotchClient/client-hoppscotch.js (3)
59-60: Correct method comment for registerMessageHandler.The method comment incorrectly states this is for registering error handlers, but it's actually for message handlers.
- // Method to register custom error handlers + // Method to register custom message handlers registerMessageHandler(handler) {
83-85: Consider JSON handling consistency with documentation.The
sendEchoMessagemethod usesJSON.stringifyon all messages, but the README example (line 53) implies sending a simple string. This inconsistency could cause confusion. Consider either:
- Updating the method to handle both string and object types, or
- Clarifying in the documentation that all messages are automatically JSON-stringified
sendEchoMessage(message) { - this.websocket.send(JSON.stringify(message)); + const payload = typeof message === 'string' ? message : JSON.stringify(message); + this.websocket.send(payload); console.log('Sent message to echo server:', message); }
77-80: Simplify the handleMessage method.The current implementation is a simple wrapper that only calls the callback if it exists. This pattern is already used in other methods like the WebSocket's onmessage handler. Consider inlining this logic to reduce unnecessary abstraction.
- // Method to handle message with callback - handleMessage(message, cb) { - if (cb) cb(message); - }And update line 35:
- this.handleMessage(event.data, handler); + handler(event.data);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/templates/clients/websocket/javascript/README.md(1 hunks)packages/templates/clients/websocket/javascript/example.js(1 hunks)packages/templates/clients/websocket/javascript/test/integration.test.js(1 hunks)packages/templates/clients/websocket/javascript/test/temp/snapshotTestResult/hoppscotchClient/README.md(1 hunks)packages/templates/clients/websocket/javascript/test/temp/snapshotTestResult/hoppscotchClient/client-hoppscotch.js(1 hunks)packages/templates/clients/websocket/javascript/test/temp/snapshotTestResult/postmanClient/README.md(1 hunks)packages/templates/clients/websocket/javascript/test/temp/snapshotTestResult/postmanClient/client-postman.js(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
packages/templates/clients/websocket/javascript/test/integration.test.js (1)
packages/templates/clients/websocket/python/test/integration.test.js (1)
path(5-5)
packages/templates/clients/websocket/javascript/test/temp/snapshotTestResult/hoppscotchClient/client-hoppscotch.js (2)
packages/templates/clients/websocket/javascript/test/temp/snapshotTestResult/postmanClient/client-postman.js (1)
WebSocket(10-10)packages/templates/clients/websocket/javascript/example.js (1)
message(26-26)
🪛 LanguageTool
packages/templates/clients/websocket/javascript/README.md
[uncategorized] ~9-~9: Did you mean: “By default,”?
Context: ...rated by the test: node example.js > By default this is testing against Postman echo se...
(BY_DEFAULT_COMMA)
[uncategorized] ~9-~9: You might be missing the article “the” here.
Context: ...ocket/javascript/example.jsand change first line toconst WSClient = require('./te...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~9-~9: Possible missing article found.
Context: ...n node example.js again. You will see example still works but now it is using a diffe...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~9-~9: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... again. You will see example still works but now it is using a different API. This i...
(COMMA_COMPOUND_SENTENCE)
packages/templates/clients/websocket/javascript/test/temp/snapshotTestResult/hoppscotchClient/README.md
[uncategorized] ~39-~39: You might be missing the article “the” here.
Context: ...Receive the timestamp message sent from server every second. Example: ```javascr...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (2)
packages/templates/clients/websocket/javascript/example.js (1)
1-1: LGTM! Import path updated to reflect new directory structure.The path to the client module has been correctly updated to match the new file structure, which aligns with the PR objective to recreate the file structure.
packages/templates/clients/websocket/javascript/test/temp/snapshotTestResult/postmanClient/README.md (1)
46-93: Good implementation of the client testing example.The example provides a comprehensive demonstration of the client usage, including message and error handling, connection establishment, and a robust approach to sending messages periodically with proper error handling.
...es/clients/websocket/javascript/test/temp/snapshotTestResult/postmanClient/client-postman.js
Outdated
Show resolved
Hide resolved
packages/templates/clients/websocket/javascript/test/integration.test.js
Outdated
Show resolved
Hide resolved
| #### `handleTimeStampMessage(payload)` | ||
| Receive the timestamp message sent from server every second. | ||
|
|
||
|
|
||
| **Example:** | ||
| ```javascript | ||
| client.handleTimeStampMessage("test"); | ||
| ``` |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Verify the implementation of handleTimeStampMessage.
The README documents a handleTimeStampMessage function, but there doesn't appear to be a corresponding implementation in the client-hoppscotch.js file. Additionally, the example shows this method being used with a string parameter "test", but the description suggests it's for receiving messages, not sending them.
🏁 Script executed:
#!/bin/bash
# Check if the handleTimeStampMessage method is implemented in the client-hoppscotch.js file
grep -n "handleTimeStampMessage" packages/templates/clients/websocket/javascript/test/temp/snapshotTestResult/hoppscotchClient/client-hoppscotch.jsLength of output: 147
Missing Implementation for handleTimeStampMessage in Client
It appears that the README documents a handleTimeStampMessage method with an example usage (client.handleTimeStampMessage("test")), yet no implementation was found in the corresponding client file. This discrepancy raises two concerns:
-
Implementation Gap:
Theclient-hoppscotch.jsfile does not contain any implementation ofhandleTimeStampMessage. Please verify if this method is defined elsewhere or if it was accidentally omitted. -
API Inconsistency:
The example passes a string parameter ("test"), which conflicts with the description that indicates the method should handle timestamp messages received from the server. Clarification is needed on whether the method is intended to process incoming messages rather than be invoked with a test string.
Please review and update the implementation or documentation accordingly.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~39-~39: You might be missing the article “the” here.
Context: ...Receive the timestamp message sent from server every second. Example: ```javascr...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
...ents/websocket/javascript/test/temp/snapshotTestResult/hoppscotchClient/client-hoppscotch.js
Outdated
Show resolved
Hide resolved
Light13008
changed the title
Light13008
force-pushed
the
postmanClient-hopscotchClient
branch
from
fd25e4b to
77a43a4
Compare
.../templates/clients/websocket/javascript/test/temp/snapshotTestResult/postmanClient/README.md
Outdated
Show resolved
Hide resolved
Light13008
force-pushed
the
postmanClient-hopscotchClient
branch
from
c6fab20 to
1337ae5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/templates/clients/websocket/javascript/README.md (1)
9-9: Refine README text for clarity and minor grammatical improvements.Consider updating this line by inserting a comma after "By default", adding the definite article "the" before "Postman echo service" and before "first line", and inserting a comma before "but" to separate independent clauses. This will improve readability and conform to standard English grammar. For example:
- > By default this is testing against Postman echo service. You can modify `packages/templates/clients/websocket/javascript/example.js` and change first line to `const WSClient = require('./test/temp/snapshotTestResult/hoppscotch-client/client-hoppscotch.js');` again. You will see example still works but now it is using a different API. This is possible since both AsyncAPI documents define the same name of operation for sending messages: `sendEchoMessage` so each client generated has the same API. + > By default, this is testing against the Postman echo service. You can modify `packages/templates/clients/websocket/javascript/example.js` and change the first line to `const WSClient = require('./test/temp/snapshotTestResult/hoppscotch-client/client-hoppscotch.js');` again. You will see that the example still works, but now it is using a different API. This is possible since both AsyncAPI documents define the same name of operation for sending messages: `sendEchoMessage`, so each client generated has the same API.🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: Did you mean: “By default,”?
Context: ...rated by the test:node example.js> By default this is testing against Postman echo se...(BY_DEFAULT_COMMA)
[uncategorized] ~9-~9: You might be missing the article “the” here.
Context: ...ocket/javascript/example.jsand change first line toconst WSClient = require('./te...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~9-~9: Possible missing article found.
Context: ...t-hoppscotch.js');` again. You will see example still works but now it is using a diffe...(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~9-~9: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... again. You will see example still works but now it is using a different API. This i...(COMMA_COMPOUND_SENTENCE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/generator/lib/generator.js(1 hunks)packages/templates/clients/websocket/javascript/README.md(1 hunks)packages/templates/clients/websocket/javascript/example.js(2 hunks)packages/templates/clients/websocket/javascript/test/integration.test.js(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/templates/clients/websocket/javascript/example.js
- apps/generator/lib/generator.js
- packages/templates/clients/websocket/javascript/test/integration.test.js
🧰 Additional context used
🪛 LanguageTool
packages/templates/clients/websocket/javascript/README.md
[uncategorized] ~9-~9: Did you mean: “By default,”?
Context: ...rated by the test: node example.js > By default this is testing against Postman echo se...
(BY_DEFAULT_COMMA)
[uncategorized] ~9-~9: You might be missing the article “the” here.
Context: ...ocket/javascript/example.jsand change first line toconst WSClient = require('./te...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~9-~9: Possible missing article found.
Context: ...t-hoppscotch.js');` again. You will see example still works but now it is using a diffe...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~9-~9: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... again. You will see example still works but now it is using a different API. This i...
(COMMA_COMPOUND_SENTENCE)
Light13008
force-pushed
the
postmanClient-hopscotchClient
branch
from
c0dffa9 to
3d73066
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/templates/clients/websocket/javascript/README.md (1)
9-9: Improve Documentation Clarity & Grammar in the Example NoteThe updated example now correctly points to the
postman-clientlibrary, resolving the earlier naming discrepancy. However, there are a few grammatical improvements to boost clarity and readability:
- Insert a comma after “By default” and add the article “the” before “Postman Echo service.”
- Consider removing the word “again” after the import path instruction.
- Insert a comma before “but” in the sentence “You will see example still works but now it is using a different API.” This will better separate the independent clauses.
For instance, below is a suggested diff to refine the text:
- > By default this is testing against Postman echo service. You can modify `packages/templates/clients/websocket/javascript/example.js` and change first line to `const WSClient = require('./test/temp/snapshotTestResult/postman-client/client-postman.js');` again. You will see example still works but now it is using a different API. This is possible since both AsyncAPI documents define the same name of operation for sending messages: `sendEchoMessage` so each client generated has the same API. + > By default, this is testing against the Postman Echo service. You can modify `packages/templates/clients/websocket/javascript/example.js` and change the first line to `const WSClient = require('./test/temp/snapshotTestResult/postman-client/client-postman.js');`. You will see that the example still works, but now it uses a different API. This is possible since both AsyncAPI documents define the same operation name (`sendEchoMessage`), so each generated client has the same API.These changes will ensure the README is both technically correct and easier to understand.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: Did you mean: “By default,”?
Context: ...rated by the test:node example.js> By default this is testing against Postman echo se...(BY_DEFAULT_COMMA)
[uncategorized] ~9-~9: You might be missing the article “the” here.
Context: ...ocket/javascript/example.jsand change first line toconst WSClient = require('./te...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~9-~9: Possible missing article found.
Context: ...ient-postman.js');` again. You will see example still works but now it is using a diffe...(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~9-~9: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... again. You will see example still works but now it is using a different API. This i...(COMMA_COMPOUND_SENTENCE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/templates/clients/websocket/javascript/README.md(1 hunks)packages/templates/clients/websocket/javascript/example.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/templates/clients/websocket/javascript/example.js
🧰 Additional context used
🪛 LanguageTool
packages/templates/clients/websocket/javascript/README.md
[uncategorized] ~9-~9: Did you mean: “By default,”?
Context: ...rated by the test: node example.js > By default this is testing against Postman echo se...
(BY_DEFAULT_COMMA)
[uncategorized] ~9-~9: You might be missing the article “the” here.
Context: ...ocket/javascript/example.jsand change first line toconst WSClient = require('./te...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~9-~9: Possible missing article found.
Context: ...ient-postman.js');` again. You will see example still works but now it is using a diffe...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~9-~9: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... again. You will see example still works but now it is using a different API. This i...
(COMMA_COMPOUND_SENTENCE)
|
|
LGTM 👍 |
Description
-This PR follows the new file structure and all the necessary endpoints were updated.
-Only updated for javascript.
-
postman-clientandhoppscotch-clientwere added with respectiveREADME.md-To work on echo we first need to adapt to the new file structure.
-Old pr made on this had to be canceled because of sudden update in file structure.
Fixes #1373
Related issue(s)
Fixes #1373Summary by CodeRabbit