fix: fix regression from mqttjs#1401 and allow CI test failures to break gitthub workflow (mqttjs#1443)

BertKleewein · web-flow · commit accd78e38aa8 · 2022-03-14T15:53:53.000-07:00
* fix: fix regression from mqttjs#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
diff --git a/.github/workflows/mqttjs-test.yml b/.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"
diff --git a/lib/client.js b/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
