Closed nwalters512 closed 11 months ago
@aearly done! I agree it's best to preserve compatibility with things that users may have been doing.
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?
@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:
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.
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.
Can you rebase again?
@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.
Yes, at this point you're better off adding it to your PR.
@aearly done in 8521e9d, could you re-approve the workflow run?
Published in v3.2.5
Many thanks for the quick release!
Many thanks, great work!
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));
@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?
I've opened a new one. Many thanks.
This PR specifically addresses the case of an
AggregateError
being thrown, though it'll also handle the case of anyError
object with an emptymessage
.When an
AggregateError
is constructed with only a single argument, itsmessage
property is the empty string. This means that theerr.message
check fails, and theAggregateError
is passed to theError
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 anError
instance. An alternative would be to check for the presence oferr.errors
, which would specifically handle the case oferr
being anAggregateError
.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 anError
with an empty message is propagated back to the caller without being wrapped in another instance ofError
.