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.

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(types): subscribe definition #1527

Merged
merged 1 commit into from
Jul 14, 2023
Merged

Conversation

redboltz
Copy link
Contributor

TypeScript definition of subscribe() has two functions.

public subscribe (topic:
string
| string[], opts: IClientSubscribeOptions, callback?: ClientSubscribeCallback): this

The type of the first parameter is string or string[].
The second parameter is opts. opts could have properties. e.g.)SubscriptionIdentifier

public subscribe (topic:
string
| string[]
| ISubscriptionMap, callback?: ClientSubscribeCallback): this

The type of the first parameter is string, string[], or ISubscriptionMap.
ISubscriptionMap is defined as follows:
export interface ISubscriptionMap {
/**
* object which has topic names as object keys and as value the options, like {'test1': {qos: 0}, 'test2': {qos: 2}}.
*/
[topic: string]: {
qos: QoS,
nl?: boolean,
rap?: boolean,
rh?: number
}
}

MQTT spec defines properties are in common for all subscription entries on one subscription.
So ISubscriptionMap doesn't have properties.

When ISubscriptionMap is passed as the first argument, then the following code is processed:

MQTT.js/lib/client.js

Lines 760 to 780 in aa49aaf

Object
.keys(obj)
.forEach(function (k) {
debug('subscribe: object topic %s', k)
if (!Object.prototype.hasOwnProperty.call(that._resubscribeTopics, k) ||
that._resubscribeTopics[k].qos < obj[k].qos ||
resubscribe) {
const currentOpts = {
topic: k,
qos: obj[k].qos
}
if (version === 5) {
currentOpts.nl = obj[k].nl
currentOpts.rap = obj[k].rap
currentOpts.rh = obj[k].rh
currentOpts.properties = opts.properties
}
debug('subscribe: pushing `%s` to subs list', currentOpts)
subs.push(currentOpts)
}
})

qos, nl, rap, and rh are extraced from ISubscriptionMap, but propertiy such as SubscriptionIdentifier are gotten from opts. Because properties are in common for subscribe entries.

However, the TypeScript definition doesn't have a combination of ISubscriptionMap and opts (for properties).
So this PR add it to the following definition:

public subscribe (topic:
string
| string[], opts: IClientSubscribeOptions, callback?: ClientSubscribeCallback): this

The type of opts is IClientSubscribeOptions that is defined as follows:

export interface IClientSubscribeOptions {
/**
* the QoS
*/
qos: QoS,
/*
* no local flag
* */
nl?: boolean,
/*
* Retain As Published flag
* */
rap?: boolean,
/*
* Retain Handling option
* */
rh?: number,
/*
* MQTT 5.0 properies object of subscribe
* */
properties?: {
subscriptionIdentifier?: number,
userProperties?: UserProperties
}
}

When opts is used with ISubscriptionMap, only properties field is required. So I added IClientSubscribeProperties.

@codecov
Copy link

codecov bot commented Aug 19, 2022

Codecov Report

Patch and project coverage have no change.

Comparison is base (d71b000) 85.74% compared to head (7e9089d) 85.74%.

❗ Current head 7e9089d differs from pull request most recent head 80f33c0. Consider uploading reports for the commit 80f33c0 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1527   +/-   ##
=======================================
  Coverage   85.74%   85.74%           
=======================================
  Files          13       13           
  Lines        1249     1249           
=======================================
  Hits         1071     1071           
  Misses        178      178           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

ogis-yamazaki added a commit to ogis-yamazaki/MQTT.js that referenced this pull request Aug 24, 2022
ogis-yamazaki added a commit to ogis-yamazaki/MQTT.js that referenced this pull request Aug 25, 2022
@robertsLando robertsLando changed the title Fix TypeScript subscribe definition. fix(types): subscribe definition. Jun 23, 2023
/*
* MQTT 5.0 properies object of subscribe
* */
properties?: {
Copy link
Member

Choose a reason for hiding this comment

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

Are we actually handling this somewhere in code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does "in code" mean? If it means MQTT.js internal code, the answer is no.
However, this is a part of public APIs. MQTT.js users that access MQTT.js APIs via TypeScript require it.
For example, my project using MQTT.js via TypeScript requires setting SubscriptionIdentifier property when subscrine() API calling. Lack of this TypeScript definition, I need to use type unsafe JavaScript code directly.
This definition improve type safety for TypeScript users.

@robertsLando robertsLando changed the title fix(types): subscribe definition. fix(types): subscribe definition Jun 27, 2023
Add missing ISubscriptionMap for multi entries subscribe.
Add IClientSubscribeProperties to support muptiple subscribe entries
that have individual options (qos, nl, rap, and rh) and in common
properties (subscriptionIdentifier, userProperty).
@robertsLando robertsLando merged commit debb7d9 into mqttjs:main Jul 14, 2023
4 checks passed
@redboltz redboltz deleted the fix_ts_subscribe branch July 14, 2023 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants