Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cli): supports ws and wss protocols #1310

Merged
merged 1 commit into from
May 25, 2023
Merged

feat(cli): supports ws and wss protocols #1310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions cli/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,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')
Expand Down Expand Up @@ -142,6 +143,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')
Expand Down Expand Up @@ -229,6 +231,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')
Expand Down Expand Up @@ -295,6 +298,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')
Expand Down Expand Up @@ -391,6 +395,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')
Expand Down Expand Up @@ -476,6 +481,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')
Expand Down Expand Up @@ -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')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
9 changes: 8 additions & 1 deletion cli/src/lib/pub.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,14 @@ const send = (
} else {
basicLog.published()
}
client.end()
// FIXME: When using the ws and wss protocols to connect, and QoS is 0, the message may not have been successfully sent when the publish callback is triggered. Therefore, delay closing the connection for 2 seconds.
if (['ws', 'wss'].includes(connOpts.protocol ?? '') && pubOpts.opts.qos === 0) {
setTimeout(() => {
client.end()
}, 2000)
} else {
client.end()
}
})
})
client.on('error', (err) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review:

  1. 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.

  2. The code looks well formatted and clear. It also uses the correct indentation and spacing.

  3. 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.

  4. The code also checks for the 'qos' property of the 'pubOpts' object and sets the timeout accordingly.

  5. The code uses the correct syntax for the setTimeout() function.

Overall, the code looks good and follows the best practices.

Expand Down
1 change: 1 addition & 0 deletions cli/src/types/global.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ declare global {
username?: string
password?: string
protocol?: Protocol
path?: string
key?: string
cert?: string
ca?: string
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
6 changes: 4 additions & 2 deletions cli/src/utils/parse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ const parseNumber = (value: string) => {
}

const parseProtocol = (value: string) => {
if (!['mqtt', 'mqtts'].includes(value)) {
signale.error('Only mqtt and mqtts are supported.')
if (!['mqtt', 'mqtts', 'ws', 'wss'].includes(value)) {
signale.error('Only mqtt, mqtts, ws and wss are supported.')
process.exit(1)
}
return value
Expand Down Expand Up @@ -156,6 +156,7 @@ const parseConnectOptions = (
username,
password,
protocol,
path,
key,
cert,
ca,
Expand Down Expand Up @@ -190,6 +191,7 @@ const parseConnectOptions = (
username,
password,
protocol,
path,
reconnectPeriod,
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down