[Discuss] Standardize API method error handling

[marcaaron]

Problem

There is a lack of consistency in how errors are handled in the codebase e.g. take the following examples:

Throwing error for handling in the catch

App/src/libs/API.js

Lines 284 to 289 in 370869f

	.then((response) => {
	// If authentication fails throw so that we hit
	// the catch below and redirect to sign in
	if (response.jsonCode !== 200) {
	throw new Error(response.message);
	}

Showing a growl

App/src/libs/actions/Policy.js

Lines 103 to 111 in 370869f

	return API.Policy_Create({type: CONST.POLICY.TYPE.FREE, policyName: name})
	.then((response) => {
	Onyx.set(ONYXKEYS.IS_CREATING_WORKSPACE, false);
	if (response.jsonCode !== 200) {
	// Show the user feedback
	const errorMessage = Localize.translateLocal('workspace.new.genericFailureMessage');
	Growl.error(errorMessage, 5000);
	return;
	}

Throwing an error with no message at all

App/src/libs/actions/Policy.js

Lines 345 to 349 in 370869f

	API.UpdatePolicy({policyID, value: JSON.stringify(values), lastModified: null})
	.then((policyResponse) => {
	if (policyResponse.jsonCode !== 200) {
	throw new Error();
	}

Storing the error in Onyx to show inline

App/src/libs/actions/User.js

Lines 47 to 53 in 370869f

	return API.ChangePassword({oldPassword, password})
	.then((response) => {
	if (response.jsonCode !== 200) {
	const error = lodashGet(response, 'message', 'Unable to change password. Please try again.');
	Onyx.merge(ONYXKEYS.ACCOUNT, {error});
	return;
	}

Inconsistently checking if jsonCode === 200 vs. jsonCode !== 200

App/src/libs/actions/Policy.js

Lines 305 to 311 in 370869f

	.then((data) => {
	// Save the personalDetails for the invited user in Onyx
	if (data.jsonCode === 200) {
	Onyx.merge(ONYXKEYS.PERSONAL_DETAILS, PersonalDetails.formatPersonalDetails(data.personalDetails));
	Navigation.goBack();
	return;
	}

Why is this important?

In general, keeping our code consistent makes it easier to understand, write, review, etc and will save both time and mental energy. Having a standard way to distinguish between API calls returning non 200 jsonCode and network errors means there's no question about the correct way and no discussion required.

Solution

I'm not sure yet. Let's discuss it. I think at a minimum we should decide some basic things like:

• Always check jsonCode !== 200 and not jsonCode === 200 (or vice versa)
• Generically handle fetch failures in some way to start
• Decide whether to handle non 200 jsonCode in the .then() or to always handle them in the .catch() (e.g. if we decide to handle them in the .catch() then it might make sense to throw a non 200 jsonCode at some much lower level so we do not have to explicitly throw them)

[marcaaron]

cc @parasharrajat as we kinda got sidetracked by this in a review

[parasharrajat]

Yup, I agree. This should be consistent across the codebase.

I am in favor of point 3 above.

[flodnv]

We should also consider what to do in order to handle jsonCode:666 errors everywhere, in which our API returns a user-friendly error message that can be shown to the user. See #7229 for example.

[MelvinBot]

@marcaaron, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

[marcaaron]

I think we're gonna cover this is the Offline Doc but gonna let Melv close this one since we'll probably create a new GH issue eventually.