-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Fix #1524 and #1525. #1526
Fix #1524 and #1525. #1526
Conversation
`topicAliasMaximum` for receiving is designed as setting by MqttClient option, not setting by CONNECT property. And it is automatically used as CONNECT property. However, the document said `topicAliasMaximum` is set by CONNECT property. It is bad so I fixed it. In addition, corresponding typescript definitions are fixed.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1526 +/- ##
==========================================
- Coverage 86.21% 86.18% -0.04%
==========================================
Files 12 13 +1
Lines 1306 1310 +4
==========================================
+ Hits 1126 1129 +3
- Misses 180 181 +1
☔ View full report in Codecov by Sentry. |
I have implemented TopicAlias functionality as #1301.
But I forgot to move TopicAliasMaximum property of CONNECT packet is designed as generated by options. So the document is now matched to the design. In addtion, typescript typedefinition is fixed. |
By looking at the code at: Line 490 in 3b2e1cb
If you checked few lines above the packet is created by cloning options: Line 483 in 3b2e1cb
Seems that it's correct to have it under properties otherwise that property would not be picked up by mqtt-packet: https://github.com/mqttjs/mqtt-packet/blob/master/types/index.d.ts#L69 Also by looking at the tests regarding topicAliasMaximum I found there where some tests using it at root in option and others having it inside options.properties, see #1612. If you wish to change this you should also fix the connect packet that actually clones the options like I said before, BTW I suggest to keep it like it is now |
Thanks, I will catch up the current main branch, and then I would update the PR. |
Now, I noticed that adding the following options for typescript is implemented and merged as #1525
Also I belive that I understand the design policy for
I think that if the poliy is explicitly stated somewhere, it would help other contributors. So I close the PR. |
topicAliasMaximum
for receiving is designed as setting by MqttClientoption, not setting by CONNECT property.
And it is automatically used as CONNECT property.
However, the document said
topicAliasMaximum
is set by CONNECTproperty.
It is bad so I fixed it.
In addition, corresponding typescript definitions are fixed.