mqttjs / MQTT.js

The MQTT client for Node.js and the browser
Other
8.55k stars 1.41k forks source link

Drop `connect` #1648

Open robertsLando opened 1 year ago

robertsLando commented 1 year ago

The connect logic should be moved on MqttClient constructor or a dedicated private method like setup.

ATM it is used to parse provided options and validate them, also creates a wrapper to use the correct streamBuilder based on provided protocol and support for handling servers option.

Users should just create the client instance by passing the options and then calling client.connect to connect later

Of course this would be a BREAKING CHANGE. My plan is to do all major breaking changes in v6. v6 will not be back compatible with v5 anymore but will try to fix all the problems this library has

cc @vishnureddy17 @BertKleewein opinions about this?

vishnureddy17 commented 1 year ago

If this will be included in V6, then I think we should consider making it a "bring your own connection" library. This would greatly reduce the number of options we have to deal with and allow custom connection implementations (e.g., WeChat) without polluting the project code.

However, this makes things like reconnecting tricky. We might have to push that responsibility onto the user

In my opinion, instantiation of the client should not do any I/O. It should just validate options, and the user has to call a connect() method to start the first I/O (sending the CONNECT packet)

robertsLando commented 1 year ago

However, this makes things like reconnecting tricky. We might have to push that responsibility onto the user

Nope, that should still be handled by client itself IMO. We just should move the logic there

This would greatly reduce the number of options we have to deal with and allow custom connection implementations (e.g., WeChat) without polluting the project code.

Yes but where should be put actual implementations? Because users will ask for them, we should have a place, maybe another repo or gists or I dunno whatelse

vishnureddy17 commented 1 year ago

Yes but where should be put actual implementations? Because users will ask for them, we should have a place, maybe another repo or gists or I dunno whatelse

Perhaps we can have the client be able to create the most common connections (tls, tcp, websockets), making sure that the options that are specific to the connection have a clear separation from the MQTT protocol options.

For custom connections like WeChat, I do not think we should be responsible for helping users there. If we allow "bring your own connection" we would be responsible for documenting and supporting the "bring your own connection" functionality, but WeChat would be responsible for their specific implementation details. They can still use V5 if they don't want that

robertsLando commented 1 year ago

For custom connections like WeChat, I do not think we should be responsible for helping users there. If we allow "bring your own connection" we would be responsible for documenting and supporting the "bring your own connection" functionality, but WeChat would be responsible for their specific implementation details. They can always use V5 if they don't want that

It's ok for me!

P.S: V5 is coming: https://github.com/mqttjs/MQTT.js/actions/runs/5658865891 👀

vishnureddy17 commented 1 year ago

Yay! Thanks for the hard work on this :)

github-actions[bot] commented 2 months ago

This is an automated message to let you know that this issue has gone 365 days without any activity. In order to ensure that we work on issues that still matter, this issue will be closed in 14 days.

If this issue is still important, you can simply comment with a "bump" to keep it open.

Thank you for your contribution.