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

v5 type issues #1686

Closed
dgg opened this issue Sep 6, 2023 · 4 comments
Closed

v5 type issues #1686

dgg opened this issue Sep 6, 2023 · 4 comments

Comments

@dgg
Copy link
Contributor

dgg commented Sep 6, 2023

Hello,

Our team is migrating our typescript projects from 4.3.7 to 5.0.4 and we are hitting some problems that prevent us from upgrading.

We are facing three types of problems:

  1. Some types are "missing" in the sense that they are not exported, even though they are used and, in some cases, we can make use of them, but their type cannot be exposed (like returned from a function, for instance.
  2. There is some type inconsistency with the runtime behavior. For example, an error that is set to null even though the typing specifies being Error or undefined.
  3. Other type inconsistency appears when some members that can be obtained runtime (and were exposed in previous versions) but are not declared in the types.
  4. Other inconsistency would be some functions declare callbacks in which the first error is Error | null and some other callbacks' first argument is the more standarized Error | undefined

Some examples of each problem:

import { connect,MqttClient } from "mqtt"
// missing types
import {/*IConnackPacket */ } from "mqtt"

const client: MqttClient = connect("mqtt://0.0.0.0:1883")
    // cannot type connPacket (missing exported type)
    // "usable" with anonymous func, cannot attach typed function declared somewhere else
    .on("connect", (connPacket/*: IConnackPacket*/) => {
    // ...
    })
    // cannot type msgPacket (missing exported type)
    .on("message", (topic: string, payload: Buffer, msgPacket /*: IPublishPacket*/){ }
/* type declaration in mqqtjs */
publish(topic: string, message: string | Buffer, opts?: IClientPublishOptions, callback?: DoneCallback): MqttClient;
export type DoneCallback = (error?: Error) => void;

/* client code: */
client.publish("test/topic", "hello", { qos: 2 }, (pubError?: Error, pubPacket?: unknown) => {
    if (pubError) {
       console.error("Error publishing", pubError)
    } else {
        // null pubError is returned as opposed of what the callback type specifies (undefined)
        console.info("Published", pubPacket, pubError)
    }
}) 
# running example
$ Published { cmd: 'pubrel', qos: 2, messageId: 58612 } null

With the same code from point 2), we are able to get an untypeable pubPacket on runtime. That member is not declared in the callback type.
4. Error argument is not consistent: null or undefined in the case of success.

.subscribe(topicObject: string | string[] | ISubscriptionMap, opts?: IClientSubscribeOptions | IClientSubscribeProperties, callback?: ClientSubscribeCallback): MqttClient;
export type ClientSubscribeCallback = (err: Error | null, granted: ISubscriptionGrant[]) => void;
publish(topic: string, message: string | Buffer, opts?: IClientPublishOptions, callback?: DoneCallback): MqttClient;
end(cb?: DoneCallback): MqttClient;
export type DoneCallback = (error?: Error) => void;

Of all these issues, the missing exported types (interfaces) are the ones that impact us the most as we have library function that return them for consumer usage and we are not able to achieve that because of the missing exports.

I believe it should be a lot of work to export the missing types (as they already there).

As for the mismatch between type declaration/runtime behavior I dare not to say as they probably come from other libraries,...

Long story short, we cannot move to mqttjs v5 until all types from public APIs are exported.

Thanks a lot for your great library.

@robertsLando
Copy link
Member

@dgg Could you kindly submit a PR to fix the issues please?

@dgg
Copy link
Contributor Author

dgg commented Sep 7, 2023

Sure, I will give it a try. I'll let you know how it goes.

@dgg
Copy link
Contributor Author

dgg commented Sep 7, 2023

I added a PR: #1688 that fixes some of the issues, but does not fix the inconsistencies in Error arguments for callbacks.

@robertsLando
Copy link
Member

Fixed by #1688

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

No branches or pull requests

2 participants