Ensure error objects are propagated without modification

[nwalters512]

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.

[aearly]

Can we keep the old behavior as well? I'm sure there are cases where people are throwing error-like objects that aren't actual Errors.

[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 with the AggregateError global:

async/.eslintrc.json

Line 5 in 8610499

"es6": true

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:

{
    "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?

[aearly]

Published in v3.2.5

[nwalters512]

Many thanks for the quick release!

[xescuder]

Many thanks, great work!

[xescuder]

Hi, @nwalters512. I don't know why but if you check main branch the problem is still in there. Was the merge correct?
Now I see still:
invokeCallback(callback, err && (err instanceof Error || err.message) ? err : new Error(err));

[nwalters512]

@xescuder I'd recommend opening a new issue instead of continuing conversation on this merged PR. When you open one, can you be more specific with the problem you're seeing, and maybe provide a failing test?

[xescuder]

I've opened a new one. Many thanks.