Repository: caolan/async
Pull request #1920 (2023-10-27 22:36:06)

Ensure error objects are propagated without modification
This PR specifically addresses the case of an `AggregateError` being thrown, though it'll also handle the case of any `Error` object with an empty `message`.

When an `AggregateError` is constructed with only a single argument, its `message` property is the empty string. This means that the `err.message` check fails, and the `AggregateError` is passed to the `Error` constructor, losing information about it.

I believe the `instanceof Error` check is most robust here, though it would be a change in behavior if someone was relying on being able to throw something like `{ message: 'foo' }`, i.e., something that's not actually an `Error` instance. An alternative would be to check for the presence of `err.errors`, which would specifically handle the case of `err` being an `AggregateError`.

I wrote my test such that it doesn't rely on the `AggregateError` global, which isn't available in some of the versions of Node that this repo is tested against. Instead, it checks that an `Error` with an empty message is propagated back to the caller without being wrapped in another instance of `Error`.

nwalters512: @aearly done! I agree it's best to preserve compatibility with things that users may have been doing.
aearly: Finally ran the CI pipeline, and noticed a lint error.  Apparently `AggregateError` is a relatively recent addition (node 15), and we're testing on node 12.  I update the pipeline to run on node 20, can you rebase your PR?
nwalters512: @aearly done! Could you approve the CI workflow again? That said, I think the following line might have to change to `es2021` for ESLint to be happy:

https://github.com/caolan/async/blob/8610499cd523032cc8701de4424b99bf410cc339/.eslintrc.json#L5

If you'd like to avoid that, I can rewrite this test to use a plain `Error` object. That is, I'd throw an error with an empty message and check for strict object equality. That should fail with the old code and succeed with my changes.
xescuder: Hi, any maintainer can approve this? I'm really interested. We're using external libraries that generate errors without message field (they have errorMessages, ...) and then we get a String that says `[Object object]` error, without information of the real cause.

For us it's very urgent. Thanks a lot.
aearly: Can you rebase again?
nwalters512: @aearly sorry for not providing more precise instructions: the *key* `es6` needs to change to `es2021`, not the value. That is, your `.eslintrc.json` should start with the following:

```json
    "env": {
        "browser": true,
        "node": true,
        "es2021": true
    },
```

Let me know if you'd like me to open a PR with that or incorporate it into this PR, I'd be more than happy to do so.
aearly: Yes, at this point you're better off adding it to your PR.
nwalters512: @aearly done in 8521e9d, could you re-approve the workflow run?

Patch:
diff --git a/.eslintrc.json b/.eslintrc.json
index 17eca47..e5e65bd 100644
--- a/.eslintrc.json
+++ b/.eslintrc.json
@@ -2,7 +2,7 @@
     "env": {
         "browser": true,
         "node": true,
-        "es6": "es2021"
+        "es2021": true
     },
     "parser": "@babel/eslint-parser",
     "parserOptions": {
diff --git a/lib/asyncify.js b/lib/asyncify.js
index 21eebee..13978b8 100644
--- a/lib/asyncify.js
+++ b/lib/asyncify.js
@@ -87,7 +87,7 @@ function handlePromise(promise, callback) {
     return promise.then(value => {
         invokeCallback(callback, null, value);
     }, err => {
-        invokeCallback(callback, err && err.message ? err : new Error(err));
+        invokeCallback(callback, err && (err instanceof Error || err.message) ? err : new Error(err));
     });
 }
 


Test patch:
diff --git a/test/asyncify.js b/test/asyncify.js
index f76c0a7..3417b71 100644
--- a/test/asyncify.js
+++ b/test/asyncify.js
@@ -59,6 +59,22 @@ describe('asyncify', () => {
         }
     });
 
+    it('propagates error with empty message without modification', (done) => {
+        let originalErr;
+        async.asyncify(async () => {
+            originalErr = new AggregateError([new Error('foo'), new Error('bar')]);
+            throw originalErr;
+        })((err) => {
+            assert(err);
+            assert.strictEqual(err, originalErr);
+            expect(err.errors).to.be.an('array');
+            expect(err.errors.length).to.equal(2);
+            expect(err.errors[0].message).to.equal('foo');
+            expect(err.errors[1].message).to.equal('bar');
+            done();
+        })
+    })
+
     describe('promisified', () => {
         function promisifiedTests(Promise) {
             it('resolve', (done) => {