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.

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

Thrown errors in client sagas are now logged to console. #2087

Merged
merged 1 commit into from Feb 28, 2020

Conversation

@tonyanziano
Copy link
Contributor

tonyanziano commented Feb 27, 2020

This will help debug issues like #2086

===

Due to the way sagas work, the errors thrown in our nested sagas are seen as uncaught and redux saga hides most of the internal properties of the thrown error and just shows the message, which isn't very helpful.

redux-saga provides an onError hook that you can use for catching these errors at the root level, but the onError will only get called if the caught error is of type Error. Since we defined our own error type ResponseError the onError hook never gets called. We need to update to redux-saga@1.0.0 for that check to be fixed, but this might incur some other breaking changes that I don't think this PR should deal with. See this issue for reference.

For now, we will surface the error in its entirety through console.error().

Before the change:

saga-err-before

After the change:

saga-err-after

@tonyanziano tonyanziano force-pushed the toanzian/restart-fix branch from bffe200 to 3126dd1 Feb 27, 2020
@@ -50,13 +50,18 @@ export function* throwErrorFromResponse(errorMessage: string, response: Response
status,
};
if (text) {
const errText = yield text();
const errText = yield text.call(response);

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Feb 27, 2020

Author Contributor

This was also throwing the following error:

TypeError: Failed to execute 'text' on 'Response': Illegal invocation

because text() was being called outside of the response context.

This comment has been minimized.

Copy link
@srinaath

srinaath Feb 28, 2020

Contributor

I think this is good for now. But we just do a try catch block around all our sagas and log them.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 27, 2020

Coverage Status

Coverage increased (+0.002%) to 68.165% when pulling 3126dd1 on toanzian/restart-fix into 49f2346 on master.

@@ -50,13 +50,18 @@ export function* throwErrorFromResponse(errorMessage: string, response: Response
status,
};
if (text) {
const errText = yield text();
const errText = yield text.call(response);

This comment has been minimized.

Copy link
@srinaath

srinaath Feb 28, 2020

Contributor

I think this is good for now. But we just do a try catch block around all our sagas and log them.

@tonyanziano

This comment has been minimized.

Copy link
Contributor Author

tonyanziano commented Feb 28, 2020

@srinaath once we bump react-saga to 1.0.0+ we can use the top-level onError hook to accomplish this

https://redux-saga.js.org/docs/basics/ErrorHandling.html

@tonyanziano tonyanziano merged commit 6b8b532 into master Feb 28, 2020
2 checks passed
2 checks passed
Emulator-CI-PR #109153 succeeded
Details
license/cla All CLA requirements met.
Details
@tonyanziano tonyanziano deleted the toanzian/restart-fix branch Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.