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

Fix Viewer Crashing on Load of Wrong OSCAL Doc Type #267

Merged
merged 6 commits into from
Feb 22, 2022

Conversation

zclarkEDC
Copy link
Member

Fix for #240

Should work for Catalogs, SSPs, and Components.

kylelaker and others added 2 commits February 10, 2022 20:48
This adds a generic ErrorBoundary which supports catching errors that
weren't previously being caught using the `onError` function/prop
previously. The ErrorBoundary concept, as of React 16, is the preferred
means to set a boundary for handling an error (as it will default to
unmounting the entire app). We, here, just display the generic "Yikes!"
text. Theoretically, we could export `ErrorBoundary`, make it a little
more generic, and use it all across the app (one could imagine rending a
catalog even if we for some reason fail to render a single Back Matter
item).

This is a little different than most our components as we tend to prefer
functional components but the only documentation for an ErrorBoundary
shows it as a class component.
Removed uses of setError and onError in the OSCALLoader function,
OSCALCatalogLoader, OSCALSSPLoader, and OSCALComponentLoader. The
appropriate error handling logic is covered in the ErrorBoundary class.
@kylelaker
Copy link
Contributor

Should we move ErrorBoundary into it's own file and export it so that we can have other error boundaries all over?

@kylelaker
Copy link
Contributor

That would probably also mean that ErrorBoundary should accept an errorFormatter prop or something like that so that you can override how an error is displayed at a specific part of the application:

snippets of changed functions:

function defaultFormatter(error) {
  return (
    <Alert severity="error">
      Yikes! Something went wrong loading the OSCAL data. Sorry, we&apos;ll look
      into it. ({error.message})
    </Alert>
  );
);

export class ErrorBoundary extends React.Component {
  constructor(props) {
    super(props)
    this.errorFormatter = props.errorFormatter ?? defaultFormatter;
    this.state = { error: undefined };
  }

  render() {
    if (this.state.error !== undefined) {
      return this.errorFormatter(this.state.error);
    }
    return this.props.children;
  }
}

Idk if the errorFormatter should go on this or this.state. I also don't know if I like that name.

As per @kylelaker's PR review, moving ErrorBoundary into it's own file
so it can be used else where in the future.
src/components/ErrorBoundary.js Outdated Show resolved Hide resolved
src/components/ErrorBoundary.js Outdated Show resolved Hide resolved
src/components/ErrorBoundary.js Outdated Show resolved Hide resolved
Co-authored-by: Pranav A. Kothare <pranavkothare@live.com>
@kylelaker kylelaker merged commit 21a57cb into develop Feb 22, 2022
@kylelaker kylelaker deleted the feature/doc-type-crash branch February 22, 2022 22:02
kylelaker added a commit that referenced this pull request Mar 1, 2022
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
kylelaker added a commit that referenced this pull request Mar 1, 2022
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
kylelaker added a commit that referenced this pull request Mar 1, 2022
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
kylelaker added a commit that referenced this pull request Mar 1, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants