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.

Sign up for GitHub

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

Jump to bottom

change(api-client-core): Error definitions #183

Merged
merged 1 commit into from
May 2, 2023
Merged

change(api-client-core): Error definitions #183

merged 1 commit into from
May 2, 2023

Conversation

MikePresman
Copy link
Contributor

@MikePresman MikePresman commented May 1, 2023 •
edited
Loading

Changing the error definitions and added missing attributes for errors coming out of the client. We're doing this because of the errors already defined e.g. GadgetInternalError weren't being used in appropriate spots.

PR Checklist

  • Important or complicated code is tested
  • Any user facing changes are documented in the Gadget-side changelog
  • Any immediate changes are slated for release in Gadget via a generated package dependency bump
  • Versions within this monorepo are matching and there's a valid upgrade path

@MikePresman MikePresman changed the title change(error): Change the error definitions change(api-client-core): Error definitions May 1, 2023
@MikePresman MikePresman force-pushed the change/errordefiitions branch 3 times, most recently from 9bb7bd7 to 1950097 Compare May 2, 2023 13:23
MikePresman
MikePresman commented May 2, 2023
View reviewed changes
packages/api-client-core/src/support.ts Outdated
@@ -218,7 +234,7 @@ export const getNonNullableError = (response: Result & { fetching: boolean }, da
`Internal Error: Gadget API didn't return expected data. Nothing found in response at ${dataPath.join(".")}`
);
} else if (result === null || (throwOnEmptyData && Array.isArray(dataArray) && dataArray.length === 0)) {
return new GadgetInternalError(`Internal Error: Gadget API returned no data at ${dataPath.join(".")}`);
return new GadgetNotFoundError(`Internal Error: Gadget API returned no data at ${dataPath.join(".")}`);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

js-clients/packages/api-client-core/src/support.ts

Lines 155 to 160 in 1950097

/**
* A Gadget API error that represents an error where the client asked the server for data that doesn't exist server side.
*/
export class GadgetNotFoundError extends Error {
code = "GGT_RECORD_NOT_FOUND";
}

@MikePresman MikePresman marked this pull request as draft May 2, 2023 13:39
@MikePresman MikePresman marked this pull request as ready for review May 2, 2023 13:45
@MikePresman MikePresman force-pushed the change/errordefiitions branch 7 times, most recently from 0b12975 to 69d59ba Compare May 2, 2023 14:56
scott-rc
scott-rc reviewed May 2, 2023
View reviewed changes
packages/api-client-core/src/support.ts Outdated
/** @private */
statusCode = 500;
/** @private */
causedByClient = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be false?

Copy link
Contributor Author

@MikePresman MikePresman May 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My rationale was that if there was a GadgetInternalError then the client (The gadget platform) is at fault here - i.e. "caused" this error to be thrown. My interpretation could be wrong. Lmk if you disagree

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, oh man I have no idea what causedByClient means anymore. You could very well be correct, but GadgetClientError having causedByClient = false and GadgetInternalError having causedByClient = true makes my brain 🤯

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My rationale was that if there was a GadgetInternalError then the client (The gadget platform) is at fault here - i.e. "caused" this error to be thrown. My interpretation could be wrong. Lmk if you disagree

I think you are getting this backwards.

The gadget client is this code, the js-client.

causedByClient can be reworded as a 4xx HTTP status code, it means incorrect args were sent from the client. And we should not report this to sentry. An example is findOne(...) for an ID that doesn't exist.

GadgetInternalError means the Gadget servers returned invalid data, in that case causedByClient = false.

packages/api-client-core/src/support.ts Outdated Show resolved Hide resolved
@MikePresman MikePresman requested a review from scott-rc May 2, 2023 19:22
scott-rc
scott-rc approved these changes May 2, 2023
View reviewed changes
Copy link
Contributor

@scott-rc scott-rc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@MikePresman MikePresman merged commit 3144ce1 into main May 2, 2023
@MikePresman MikePresman deleted the change/errordefiitions branch May 2, 2023 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scott-rc scott-rc scott-rc approved these changes

@angelini angelini angelini approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MikePresman @angelini @scott-rc