feat(cli): supports ws and wss protocols #1310
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -574,6 +580,7 @@ export class Commander { | |||
| .option('-u, --username <USER>', 'the username') | |||
| .option('-P, --password <PASS>', 'the password') | |||
| .option('-l, --protocol <PROTO>', 'the protocol to use, mqtt or mqtts', parseProtocol, 'mqtt') | |||
| .option('--path <PATH>', 'the path of websocket', '/mqtt') | |||
| .option('--key <PATH>', 'path to the key file') | |||
| .option('--cert <PATH>', 'path to the cert file') | |||
| .option('--ca <PATH>', 'path to the ca certificate') | |||
There was a problem hiding this comment.
the code review.
Firstly, I recommend you to consider using DRY principle (Don't Repeat Yourself). It looks like you have repeated the same code several times, which is unnecessary and can cause errors if you need to make changes later on.
Secondly, it's important to make sure that all the options are properly documented and the default values are clearly defined. As you have already done this, I suggest a more detailed documentation for the '--path ' option.
Thirdly, it would be beneficial to consider adding some error handling code to your code in order to prevent any unexpected errors.
Finally, I suggest that you should test your code thoroughly in order to ensure that it works properly.
I hope these suggestions help you improve your code patch.
| }, 2000) | ||
| } else { | ||
| client.end() | ||
| } | ||
| }) | ||
| }) | ||
| client.on('error', (err) => { |
There was a problem hiding this comment.
the code review:
-
The code patch provides a solution to delay closing the connection when using the ws and wss protocols to connect and QoS is 0. This is to ensure that the message is successfully sent before the connection is closed.
-
The code looks well formatted and clear. It also uses the correct indentation and spacing.
-
The code uses a ternary operator to check for the value of the 'protocol' property in the 'connOpts' object, and if it is not available, then it defaults to an empty string.
-
The code also checks for the 'qos' property of the 'pubOpts' object and sets the timeout accordingly.
-
The code uses the correct syntax for the setTimeout() function.
Overall, the code looks good and follows the best practices.
| @@ -23,6 +23,7 @@ declare global { | |||
| username?: string | |||
| password?: string | |||
| protocol?: Protocol | |||
| path?: string | |||
| key?: string | |||
| cert?: string | |||
| ca?: string | |||
There was a problem hiding this comment.
This code patch looks correct, but there are a few things to consider.
First, the new addition of the "path" variable should be declared with a type, such as string or number, to ensure that it is used correctly.
Second, it would be best to add a comment explaining the purpose of the added variable, for future maintainers.
Third, the global scope could potentially be too broad for the variable's use case. Consider if there are other scopes, such as a function or class, that the variable should be declared in instead.
Finally, a test should be written to ensure that the variable is working as expected.
Overall, this code patch looks good but it would be beneficial to add more details and tests before implementing it in production.
| @@ -190,6 +191,7 @@ const parseConnectOptions = ( | |||
| username, | |||
| password, | |||
| protocol, | |||
| path, | |||
| reconnectPeriod, | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
with the code review.
The first part of the patch changes the error message that is displayed when an invalid protocol is entered. This is a good change, as it makes the error message more informative. The second part adds a new parameter, path, to the parseConnectOptions function. This is also a good change, as it allows the user to specify a custom path for their connection. Overall, the patch looks good and should be accepted.
PR Checklist
If you have any questions, you can refer to the Contributing Guide
What is the current behavior?
Please describe the current behavior and link to a relevant issue.
Issue Number
Example: #123
What is the new behavior?
Please describe the new behavior or provide screenshots.
Does this PR introduce a breaking change?
Specific Instructions
Are there any specific instructions or things that should be known prior to review?
Other information