fetch calls are too optimistic

[peterbe]

This is definitely a feature that can wait but the fetch calls all assume that JSON is returned.

delivery-dashboard/src/PollbotAPI.js

Lines 15 to 34 in 900b00e

	const response = await fetch(`${SERVER}/${product}/ongoing-versions`);
	return response.json();
	}
	export async function getReleaseInfo(
	product: Product,
	version: string,
	): Promise<ReleaseInfo> {
	const response = await fetch(`${SERVER}/${product}/${version}`);
	return response.json();
	}
	export async function checkStatus(url: string): Promise<CheckResult> {
	const response = await fetch(url);
	return response.json();
	}
	export async function getPollbotVersion(): Promise<APIVersionData> {
	const response = await fetch(`${SERVER}/__version__`);
	return response.json();

If anything goes wrong with the request, there might not be a valid JSON and you'd get a cryptic error.
Instead of

const response = await fetch(url);
return response.json();

we should be doing something like:

const response = await fetch(url);
if (response.status === 200) {
  return response.json();
} else {
  return Promise.reject(response.status);
}

Example

This way, we could do something more useful if ANY of the XHR fails.

[magopian]

I could be totally wrong, but I thought that the current code would result in an error (response.json() would throw), and thus be caught by the caller: https://github.com/mozilla/delivery-dashboard/blob/master/src/sagas.js#L158-L163

[peterbe]

That sagas.js code is way too broad. It looks for anything that could go wrong anywhere in that huge block.

(Personally, a complete sagas noob)

Also, what good is a console.error to a user :)
If pollbot is down, or even more likely, your own internet connection is down because your kids ate the wifi router, you'd get no feedback about the app not being able to talk to the internet.

I don't have a good solution (even though I think I know what the steps are) but ideally if a network fetch fails it shouldn't break things but you should get a nice human alert at the top of the page saying it can't proceed because of a network issue. Plus the message should somehow indicate that you can probably just reload/refresh the page and try again. Kinda like Gmail does.

[magopian]

Yeah the console.error was meant to be temporary and an added information when debugging (like it's done some other sagas where you would also display a nice error to the user like in https://github.com/mozilla/delivery-dashboard/blob/master/src/sagas.js#L121-L122).

I agree it would make sense to have a "finer" error handling, and better errors displayed to the user 👍

[peterbe]

In Tecken, if any fetch() call yields a response whose status != 200, I set the response into a MobX global state store and the top-level App component displays a little alert box (independent of which page you're on) if there was a failed response. It worked pretty well but kinda sucky because you might not know what thing on the page was related to that network call. Also, some network calls aren't important. And in hindsight, I shouldn't have just 1 time in the MobX store for failed responses but it would be better with a list. Then you can display one alert error message per URL maybe.

Also, my solution required manual code in every place the fetch is called. E.g. for example and that was kinda messy and I'm sure I've forgotten to do it in some places.