fix: fix regression from #1401 and allow CI test failures to break gi…

…tthub workflow (#1443)

* fix: fix regression from #1401 and allow CI test failures to break github workflow

* temporary rollback to verify break works

* put change back in

* add comment to fix

* fix misspelling

.github/workflows/mqttjs-test.yml

@@ -30,7 +30,6 @@ jobs:
     - run: npm ci
     - name: Node Tests
       run: npm run test:node
-      continue-on-error: true
       env:
         CI: true
         DEBUG: "mqttjs"

lib/client.js

@@ -1583,7 +1583,18 @@ MqttClient.prototype._handleAck = function (packet) {
   const that = this
   let err
 
-  if (!cb || cb === nop) {
+  // Checking `!cb` happens to work, but it's not technically "correct".
+  //
+  // Why? This code assumes that "no callback" is the same as that "we're not
+  // waiting for responses" (puback, pubrec, pubcomp, suback, or unsuback).
+  //
+  // It would be better to check `if (!this.outgoing[messageId])` here, but
+  // there's no reason to change it and risk (another) regression.
+  //
+  // The only reason this code works is becaues code in MqttClient.publish,
+  // MqttClinet.subscribe, and MqttClient.unsubscribe ensures that we will
+  // have a callback even if the user doesn't pass one in.)
+  if (!cb) {
     debug('_handleAck :: Server sent an ack in error. Ignoring.')
     // Server sent an ack in error, ignore it.
     return