Catch unexpected exceptions and offer to report them as bugs

[cowboyd]

closes #618

Motivation

It's a tough look whenever something terrible happens and the server or the CLI halts without doing the thing that you were asking it to do. For example, we were having an issue with our proxy server crashing on canceled requests with:

Error: unexpected end of file
    at Zlib.zlibOnError [as onerror] (zlib.js:180:17) {
  errno: -5,
  code: 'Z_BUF_ERROR'
}

It's even worse when that happens, and it silently makes a terrible impression on our users and we never even have the notification that it happened.

Approach

This change catches all top-level exceptions that are not effetion MainErrors and prints out an explanation and generates a link to a new github issue with several diagnostic values pre-filled in.

Screeshots

TODOS & Open Questions

• I'm not sure the best way to test this. I could, of course unit test the warnUnexpectedErrors() middleware directly, but It's kind of a small test given something that is so over-arching in its effect. If there were some way to booby-trap the CLI so that by running the bigtest throw command, you could have it raise an arbitrary exception from the CLI. Ideally, we could use a plugin architecture inside the test case to exend the CLI with a throw command, but that seems like a lot of work. Still, having our thing that is there to give us a good look even when we look bad, makes us look even worse.... well that is really bad. So not having this really well tested feel dangerous
• The way that this was implemented was to catch all non MainErrors, but that felt not so great. It occurred to me that what might even be better would be to have the Promise returned from @effection/nodealways resolve on both successful exits _and_ onMainError` exits since these are still expected outcomes. Only on an unexpected error would the promise actually reject. This would have the benefit of allowing custom error handling of unexpected exits, but baked in handling of all expected exits. In other words, we would have been able to write this as:

import { CLI } from './cli';
import { main } from '@effection/node';
import { unexpectedExit } from './unexpected-exit';

main(CLI(process.argv.slice(2)))
  .catch(unexpectedExit);

This just makes the unexpected exit code a lot easier to write since it is just a function, not an operation, and it doesn't need to know anything amout MainError

[changeset-bot]

🦋 Changeset detected

Latest commit: eb70190

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@bigtest/cli	Patch
bigtest	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

The preview packages of this pull request have been published.
Click on the following packages for instructions on how to install them:

@bigtest/agent

---

Install using the following command:

$ npm install @bigtest/agent@no-naked-errors

Or update your package.json file:

{
  "@bigtest/agent": "no-naked-errors"
}

---

bigtest

---

Install using the following command:

$ npm install bigtest@no-naked-errors

Or update your package.json file:

{
  "bigtest": "no-naked-errors"
}

---

@bigtest/cli

---

Install using the following command:

$ npm install @bigtest/cli@no-naked-errors

Or update your package.json file:

{
  "@bigtest/cli": "no-naked-errors"
}

---

@bigtest/client

---

Install using the following command:

$ npm install @bigtest/client@no-naked-errors

Or update your package.json file:

{
  "@bigtest/client": "no-naked-errors"
}

---

@bigtest/effection-express

---

Install using the following command:

$ npm install @bigtest/effection-express@no-naked-errors

Or update your package.json file:

{
  "@bigtest/effection-express": "no-naked-errors"
}

---

@bigtest/interactor

---

Install using the following command:

$ npm install @bigtest/interactor@no-naked-errors

Or update your package.json file:

{
  "@bigtest/interactor": "no-naked-errors"
}

---

@bigtest/server

---

Install using the following command:

$ npm install @bigtest/server@no-naked-errors

Or update your package.json file:

{
  "@bigtest/server": "no-naked-errors"
}

---

@bigtest/webdriver

---

Install using the following command:

$ npm install @bigtest/webdriver@no-naked-errors

Or update your package.json file:

{
  "@bigtest/webdriver": "no-naked-errors"
}

---

Generated by 🚫 dangerJS against eb70190

[jnicklas]

Very cool!

Two small tweaks we could do:

1. We could use something like https://github.com/sindresorhus/terminal-link so we don't have to show a gigantic link in the terminal.

2. We could use a <details> tag to hide the stack trace in the issue template, this will make the resulting issue look much neater.

GitHub

sindresorhus/terminal-link

Create clickable links in the terminal. Contribute to sindresorhus/terminal-link development by creating an account on GitHub.

[cowboyd]

@jnicklas done! Those are pretty cool techs. I did notice that including html comments inside the github link as I had it was breaking the terminalLink function.

[jnicklas]

@cowboyd hmm, I tried this out, and the link didn't render for me, so I tried to figure out what's going on, and then realized that unfortunately terminal links are not supported in tmux, so tmux will just show this as plain, unlinked text :( They have an open PR for it, but it seems to be stalled. tmux/tmux#2403

[cowboyd]

What do you think w.r.t. testing?

[jnicklas]

How about just unit testing this thing? That should be fine, right?

One issue that occurred to me is what happens in case of a port collision? We should handle those somewhat gracefully, imo, and not throw a screaming error in case it happens, because it really isn't an innternal failure.

[cowboyd]

One issue that occurred to me is what happens in case of a port collision? We should handle those somewhat gracefully, imo, and not throw a screaming error in case it happens, because it really isn't an innternal failure.

I agree. We're totally able to handle that, even if it means that we log an error and exit.

[cowboyd]

Oops, forgot to commit a file

[jnicklas]

How about a fix for EADDREINUSE should we put that in there?

[cowboyd]

@jnicklas I created a ticket so that we wouldn't forget #651 but there is not much chance that I'll get to it today. Can we merge this as-is and then add that fix later?

[jnicklas]

Let's go with this for now and we can fix the port issue later.