-
Notifications
You must be signed in to change notification settings - Fork 11
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
Handle errors during URL fetching #286
Conversation
An earlier version of this work did leverage e7c9642 but this current implementation doesn't. I am going to rebase this work to split that commit into its own PR and cleanup the history on this PR. |
ac8bfa7
to
d19be32
Compare
Samples of a few different error types; both ones fixed by #267 and ones that worked before (and some that worked after), some occur in the 200 with a non-JSON response ( |
d19be32
to
20dd12e
Compare
Because `OSCALLoader` itself is the top-level component that has an `<ErrorBoundary>` as a child, we cannot rely on the `<ErrorBoundary>` to catch errors further in the stack. Additionally, Error Boundaries do a poor job of catching asynchronous errors. Instead, we track error state (which we did prior to #267, though, this implementation is a little cleaner as we don't try to pass the `onError` further down the call stack since we _can_ use the `<ErrorBoundary>` for that. Since we've refactored the logic to handle the error into its own (reused) function, it's now also trivial to set the `loaded` and `isResolutionComplete` variables, which means we can also re-enable the "Reload" button even after an error. Resolves #283 Resolves #163
20dd12e
to
e738eca
Compare
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.
Nice fix!
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.
Minor, unrelated suggestion.
We actually have no means to "look into it" (right now).
This allows displaying a more friendly error text but also expanding it to view the specific details.
Because
OSCALLoader
itself is the top-level component that has an<ErrorBoundary>
as a child, we cannot rely on the<ErrorBoundary>
tocatch errors further in the stack. Additionally, Error Boundaries do a
poor job of catching asynchronous errors. Instead, we track error state
(which we did prior to #267, though, this implementation is a little
cleaner as we don't try to pass the
onError
further down the callstack since we can use the
<ErrorBoundary>
for that.Since we've refactored the logic to handle the error into its own
(reused) function, it's now also trivial to set the
loaded
andisResolutionComplete
variables, which means we can also re-enable the"Reload" button even after an error.
Resolves #283
Resolves #163