Browse Source

chore: improve existing error preservation in promisify (#16815)

This PR better preserves existing behavior in `deprecate.promisify()` in the cases where the promise fails. Previously, if a callback was only called with `data` instead of `err, data` and the promise was rejected, `data` would be populated with `err`, which could be confusing to users. This makes it such that `err` is called back on promise rejection if a callback is called with `err, data` a la Node.js.
Shelley Vohr 6 years ago
parent
commit
9ebba76c03
2 changed files with 21 additions and 1 deletions
  1. 3 1
      lib/common/api/deprecate.js
  2. 18 0
      spec/api-deprecations-spec.js

+ 3 - 1
lib/common/api/deprecate.js

@@ -89,7 +89,9 @@ const deprecate = {
             cb.length === 2 ? cb(null, res) : cb(res)
           })
         }, err => {
-          process.nextTick(() => cb(err))
+          process.nextTick(() => {
+            cb.length === 2 ? cb(err) : cb()
+          })
         })
     }
   },

+ 18 - 0
spec/api-deprecations-spec.js

@@ -145,6 +145,24 @@ describe('deprecations', () => {
       expect(warnings).to.have.lengthOf(0)
     })
 
+    it('only calls back an error if the callback is called with (err, data)', (done) => {
+      enableCallbackWarnings()
+      let erringPromiseFunc = () => new Promise((resolve, reject) => {
+        reject(new Error('fail'))
+      })
+      erringPromiseFunc = deprecate.promisify(erringPromiseFunc)
+
+      erringPromiseFunc((err, data) => {
+        expect(data).to.be.an('undefined')
+        expect(err).to.be.an.instanceOf(Error).with.property('message', 'fail')
+        erringPromiseFunc(data => {
+          expect(data).to.not.be.an.instanceOf(Error)
+          expect(data).to.be.an('undefined')
+          done()
+        })
+      })
+    })
+
     it('warns exactly once for callback-based invocations', (done) => {
       enableCallbackWarnings()
       promiseFunc = deprecate.promisify(promiseFunc)