Re-throw errors if we're not being specific when catching them

[armenzg]

In this change [1] I catch an error with typeof and e.message to be sure it is the one I want to catch.
We have a lot of code catching errors, however, if we're not being specific we can hide other errors.

I would like this issue to:

• Find all instances of catching errors that are not being specific
• If we cannot easily cause the errors and make the catch more specific we should add throw error and let it get to the console and sentry.io

[1]

firefox-code-coverage-frontend/src/components/diffviewer.js

Lines 47 to 60 in 135da3f

	} catch (e) {
	if ((e instanceof TypeError) && (e.message === 'Failed to fetch')) {
	this.setState({
	appError: 'We\'ve had a network issue. Please try again.',
	});
	} else {
	this.setState({
	appError: 'We did not manage to parse the diff correctly.',
	});
	// Since we're not checking for e.message we should raise it
	// so it shows up on sentry.io
	throw e;
	}
	}

[arushnagpal]

I would like to contribute to this issue, Can I help?

[armenzg]

Sure! Have a look at the contribution guide and look for try/catch blocks on the code.
https://github.com/mozilla/firefox-code-coverage-frontend/blob/master/CONTRIBUTING.md

Thanks!

[armenzg]

Could you also see to introduce an error boundary wrapping the whole app?
https://reactjs.org/blog/2017/07/26/error-handling-in-react-16.html#introducing-error-boundaries

Probably replacing it this div:
https://github.com/mozilla/firefox-code-coverage-frontend/blob/master/src/components/app.js#L39
You will probably need to create an error_boundary.jsx file.

The call to logErrorToMyService should probably be this:
https://docs.sentry.io/clients/javascript/#manually-reporting-errors

[ghost]

Hello @armenzg can I work on this issue?

[armenzg]

Hey @csd713 ! sure thing.
Does the first comment make sense to you?

[ghost]

Not really :) Could you please give a little more info on this?

[armenzg]

If you look at this change you might be able to understand what I'm saying.

This code can fail either fetching or parsing what was fetched:

const text = await (await FetchAPI.getDiff(changeset)).text();
this.setState({ parsedDiff: parse(text) });

Now, a network error was happening, however, we were printing this message We did not manage to parse the diff correctly.
We were also not re-raising the errors which means that we were not sending this error to sentry.io (uncaught errors are reported to sentri.io).

I modified the code to make it distringuish the network error but using throw e to let it bubble up and end up being reported in sentri.io.

If you read my comment when filing this issue you will see that in most cases all we need is add a throw e in the catch statements. In some cases we might be able to use e instanceOf Foo && e.message === 'bar'.

I'm also going to request this issue to become available for takes:
#96
There's also this if you're interested on a bug that will require some fair amount of debugging:
#135

On another note, I'm going to be starting a new project in the next couple of weeks in case you want to work with me on it. In short, it will be a rewrite of this site: https://arewefastyet.com
My email is armenzg@mozilla.com if you're interested.

[armenzg]

Fixed by @csd713 ! 🎉 🎆