-
Notifications
You must be signed in to change notification settings - Fork 52
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.
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
Use Result type and throw fewer errors in Client and Core #1277
Conversation
…nt-core # Conflicts: # packages/js/test-env/src/index.ts
This pull request fixes 1 alert when merging 86caf5a into 3cb0263 - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging ca10282 into 3cb0263 - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging fa6c52d into 3cb0263 - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging 657b2da into 3bf6502 - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging 6ab5736 into 3bf6502 - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging ae28dc1 into 3bf6502 - view on LGTM.com fixed alerts:
|
@@ -202,7 +215,7 @@ export class PolywrapClient implements Client { | |||
>(options: QueryOptions<TVariables, TUri>): Promise<QueryResult<TData>> { | |||
let result: QueryResult<TData>; | |||
|
|||
try { | |||
err: try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious here: What's the reason for this syntax? I am noticing that you are then using a break but we can also return { errors: [parseResult.error as Error] }
? Probably I am wrong but just wanted to understand this :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because of the clearContext
below the try catch statement. I wanted to be sure it still ran since it was running on error before the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super cool :) Feel free to ignore my comments
expect(response.data).toBeDefined(); | ||
expect(response.data).toBe(true); | ||
if (!response.ok) fail(response.error); | ||
expect(response.value).toBeDefined(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one is redundant
@krisbitney PR looks awesome! I left a couple comments and there's some minor fixes in this PR: #1285 |
…-fixes Result error handling fixes
This pull request fixes 1 alert when merging af0d730 into 3bf6502 - view on LGTM.com fixed alerts:
|
This PR replaces most thrown errors in the client and core with the
Result
pattern. Notably,client.invoke
now returns aResult<TData, Error>
.Closes #1018, #1245