chore!: remove unused deps, convert to ES2015 class #1633
Conversation
BREAKING CHANGE: when creating an `MqttClient` instance `new` is now required
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1633 +/- ##
==========================================
- Coverage 86.22% 85.74% -0.48%
==========================================
Files 13 13
Lines 1321 1249 -72
==========================================
- Hits 1139 1071 -68
+ Misses 182 178 -4
☔ View full report in Codecov by Sentry. |
robertsLando
changed the title
new required
|
@BertKleewein FYI first time tests failed for no reason (this didn't happened for long so tests are better but still flaky): https://github.com/mqttjs/MQTT.js/actions/runs/5509746410/jobs/10042923374?pr=1633#step:5:2817 |
|
Many deps are related to
So this are 5 deps on a total of 15. I would suggest to create a new dedicated repo Of course this will be done on a separeted PR |
It is not costless. There's some overhead because we have to pay attention to another repo for maintenance. I still think it's good on net, reducing the dependencies here would be great! |
| @@ -68,7 +67,7 @@ function connect (brokerUrl, opts) { | |||
| parsed.port = Number(parsed.port) | |||
| } | |||
|
|
|||
| opts = xtend(parsed, opts) | |||
There was a problem hiding this comment.
Good call to get rid of dependencies that are no longer needed due to newer JS features.
|
I have created #1634 to keep track of this. Is this good to merge so? |
|
Instead of moving bin/* to a separate repo, it could just have its own package.json. This would also be useful for samples. aedes is a devDependency, but it's only used for a single sample. This could be done lerna (another devDependency :( ) or maybe npm workspaces are mature enough for this now. update: aedes was a devDependency. I just noticed that you removed this. :) |
|
Still reviewing, almost done. |
| // Defaults | ||
| for (k in defaultConnectOptions) { | ||
| if (typeof this.options[k] === 'undefined') { | ||
| this.options[k] = defaultConnectOptions[k] | ||
| } else { | ||
| this.options[k] = options[k] | ||
| } | ||
| } |
There was a problem hiding this comment.
This might be out of scope for this PR, but it looks like this code can be simplified with spread syntax
|
I have think about npm workspaces too but introducing them requires some work to refactor all things that's why I proposed having a separeted module. Anyway I would continue this discussion on #1634 |
BREAKING CHANGE: when creating an
MqttClientinstancenewis now required