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

Ensure errors are caught and resettable #490

Merged
merged 1 commit into from
Jul 11, 2022

Conversation

kylelaker
Copy link
Contributor

We have continued to have some issues with errors that occur during the
OSCALLoader (and OSCALLoaderForm) because of the structure of various
components and where the errors occur. This implements a quick fix to
push the errors down into a child component of OSCALLoader. That
component will then throw the error, which can be caught be the
ErrorBoundary because it occurs as a child of the ErrorBoundary. This
should continue to preserve the form and other UI elements.

This has some kinda nasty side effects. The error message in the console
will sorta mask the actual location where the error occurred (it will
look like it came from the ErrorThrower) unless the error itself got
logged before that (which seems to usually happen) or unless the error
actually occurs somewhere in {result} which we've handled pretty
correctly for the most part.

ErrorThrower is deprecated upon creation because wow I really don't
want to see this in other places in our code base.

Closes #489

We have continued to have some issues with errors that occur during the
OSCALLoader (and OSCALLoaderForm) because of the structure of various
components and where the errors occur. This implements a quick fix to
push the errors down into a child component of OSCALLoader. That
component will then throw the error, which can be caught be the
ErrorBoundary because it occurs as a child of the ErrorBoundary. This
should continue to preserve the form and other UI elements.

This has some kinda nasty side effects. The error message in the console
will sorta mask the actual location where the error occurred (it will
look like it came from the `ErrorThrower`) unless the error itself got
logged before that (which seems to usually happen) or unless the error
actually occurs somewhere in `{result}` which we've handled pretty
correctly for the most part.

`ErrorThrower` is deprecated upon creation because wow I really don't
want to see this in other places in our code base.
@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team July 11, 2022 17:53
Copy link
Contributor

@Bronstrom Bronstrom left a comment

Choose a reason for hiding this comment

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

This works well! Don't see any more problems with this error when testing both the viewer and editor.

I'll admit, I'm not a huge fan of having a deprecated method, but it seems like this is one of the better options for handling the problem right now. I think the issues with state and components interacting with OSCALLoader have become more and more relevant, and I think it would make sense we try to restructure these in the near future.

@Bronstrom Bronstrom merged commit 700d221 into develop Jul 11, 2022
@Bronstrom Bronstrom deleted the feature/error-boundaries-again branch July 11, 2022 18:10
tuckerzp added a commit to EasyDynamics/oscal-editor-deployment that referenced this pull request Jul 14, 2022
tuckerzp added a commit to EasyDynamics/oscal-editor-deployment that referenced this pull request Jul 25, 2022
@kylelaker kylelaker changed the title Implement quick fix for error handling Ensure errors are caught and resettable Aug 29, 2022
@kylelaker kylelaker added the bug Something isn't working label Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error state isn't cleared when a new document is selected
3 participants