Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it more spec compliant #4

Open
BebeSparkelSparkel opened this issue Jan 10, 2018 · 15 comments
Open

Make it more spec compliant #4

BebeSparkelSparkel opened this issue Jan 10, 2018 · 15 comments

Comments

@BebeSparkelSparkel
Copy link

When used with mocha the errors are displayed twice. Is there a way to have it only display once?

const AggregateError = require('aggregate-error')

it('should fail', function() {
  throw new AggregateError([
    new Error('first'),
    new Error('second')
  ])
})

The report is

  1) should fail

  0 passing (12ms)
  1 failing

  1) should fail:
     
    Error: first
        at Context.<anonymous> (/Users/williamrusnack/Documents/event_sequencing/try_aggregate-error.js:5:5)
        at callFn (/Users/williamrusnack/Documents/event_sequencing/node_modules/mocha/lib/runnable.js:354:21)
        at Test.Runnable.run (/Users/williamrusnack/Documents/event_sequencing/node_modules/mocha/lib/runnable.js:346:7)
        at Runner.runTest (/Users/williamrusnack/Documents/event_sequencing/node_modules/mocha/lib/runner.js:442:10)
        at /Users/williamrusnack/Documents/event_sequencing/node_modules/mocha/lib/runner.js:560:12
        at next (/Users/williamrusnack/Documents/event_sequencing/node_modules/mocha/lib/runner.js:356:14)
        at /Users/williamrusnack/Documents/event_sequencing/node_modules/mocha/lib/runner.js:366:7
        at next (/Users/williamrusnack/Documents/event_sequencing/node_modules/mocha/lib/runner.js:290:14)
        at Immediate.<anonymous> (/Users/williamrusnack/Documents/event_sequencing/node_modules/mocha/lib/runner.js:334:5)
    Error: second
        at Context.<anonymous> (/Users/williamrusnack/Documents/event_sequencing/try_aggregate-error.js:6:5)
        at callFn (/Users/williamrusnack/Documents/event_sequencing/node_modules/mocha/lib/runnable.js:354:21)
        at Test.Runnable.run (/Users/williamrusnack/Documents/event_sequencing/node_modules/mocha/lib/runnable.js:346:7)
        at Runner.runTest (/Users/williamrusnack/Documents/event_sequencing/node_modules/mocha/lib/runner.js:442:10)
        at /Users/williamrusnack/Documents/event_sequencing/node_modules/mocha/lib/runner.js:560:12
        at next (/Users/williamrusnack/Documents/event_sequencing/node_modules/mocha/lib/runner.js:356:14)
        at /Users/williamrusnack/Documents/event_sequencing/node_modules/mocha/lib/runner.js:366:7
        at next (/Users/williamrusnack/Documents/event_sequencing/node_modules/mocha/lib/runner.js:290:14)
        at Immediate.<anonymous> (/Users/williamrusnack/Documents/event_sequencing/node_modules/mocha/lib/runner.js:334:5)
  AggregateError: 
      Error: first
          at Context.<anonymous> (try_aggregate-error.js:5:5)
      Error: second
          at Context.<anonymous> (try_aggregate-error.js:6:5)
      at Context.<anonymous> (try_aggregate-error.js:4:9)
@sindresorhus
Copy link
Owner

sindresorhus commented Jan 11, 2018

I have a feeling it's because of

aggregate-error/index.js

Lines 24 to 28 in 7ae39e2

* [Symbol.iterator]() {
for (const error of this._errors) {
yield error;
}
}
but I'm not sure what Mocha is doing to trigger it.

@sindresorhus
Copy link
Owner

// @boneskull

@boneskull
Copy link

good question!

@boneskull
Copy link

whoever wants to look into this, please create an issue and PR on Mocha's issue tracker. The problem is likely somewhere in this function.

@felixfbecker
Copy link
Contributor

I think one problem is that aggregate-error puts all the stacks of the errors into message, but imo message should only contain the aggregated messages. The aggregated stacks with messages could be returned when .toString() is invoked, just like with regular Errors.

@felixfbecker
Copy link
Contributor

Tested Chrome does not invoke toString() for errors, but it uses stack, and it's possible to assign this.stack to a custom concatenation of everything

@sindresorhus
Copy link
Owner

sindresorhus commented Jul 17, 2020

Yeah, I want to align this better with https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/AggregateError/AggregateError

  • Make the .message just the message of the AggregateError, and fix up the .stack.
  • Change to use an .errors property instead of the instance being iterable.
  • message argument: Add message argument #12

@sindresorhus sindresorhus changed the title Play nice with Mocha Make it more spec compliant Jul 17, 2020
@boneskull
Copy link

I'll be watching this one--Mocha should understand these errors.

@timoxley
Copy link

timoxley commented Oct 3, 2020

@sindresorhus this setup is arguably better than the spec :(

@sindresorhus
Copy link
Owner

@timoxley How so?

@boneskull
Copy link

tbh stuffing the stack(s) into message seems like misuse of the prop

@timoxley
Copy link

timoxley commented Oct 9, 2020

@sindresorhus Spec AggregateError displays ok in node because it also prints the error's own properties via util.inspect, but errors are entirely hidden in the browser console using console.log or console.error:

image


But does give better logging with console.dir

image

So I guess the problem is really more of a shortcoming of the browser console implementation rather than AggregateError itself. Nevermind

@mmkal
Copy link

mmkal commented Dec 9, 2020

@sindresorhus @timoxley I do think this is better in some ways than the spec. This library currently creates a sensible error message if a custom one isn't provided. With the official AggregateError:

image

vs current situation (and what the docs will have led users to do already in existing codebases):

image

So making it spec compliant would involve changing the message-less behaviour to make the resultant message prop less helpful. That might hurt a lot of users who auto-update when they get paged in the middle of the night with Alert! Error in production: "".

Given that, IMO making it spec compliant should be either a major release and make message required... or a non-goal - separate out functionality from the spec and rename to multi-error or something. As a user I'd prefer the latter. This library can be more useful than a polyfill (/ponyfill) of something coming to ES anyway.

@felixfbecker
Copy link
Contributor

One idea would be to turn this module into a factory function createAggregateError() for the standard global AggregateError that does the smarter error messages. It could also make sure error instances are passed. It could do more than the current one even, e.g. if only one error is passed return it as-is (don't know if that's a good idea). And then maybe provide some additional functions for dealing with AggregateErrors, e.g. a type guard isAggregateError().

@mmkal
Copy link

mmkal commented Dec 9, 2020

I found this because I was looking for something roughly equivalent to verror, which is good but hasn't been published in four years. It'd be nice to have a semi-official way of aggregating errors. But AggregateError as defined in the ES spec is a bit limited. Maybe this library could go further in the verror direction rather than towards the ES spec, and provide helpers for wrapping errors etc.?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants