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

[Docs] More guidance for EuiErrorBoundary #8379

Open
JasonStoltz opened this issue Feb 28, 2025 · 7 comments
Open

[Docs] More guidance for EuiErrorBoundary #8379

JasonStoltz opened this issue Feb 28, 2025 · 7 comments
Labels
documentation Issues or PRs that only affect documentation - will not need changelog entries feature request low hanging fruit An issue, often a bug, that is lower effort and clearly ought to be fixed

Comments

@JasonStoltz
Copy link
Member

JasonStoltz commented Feb 28, 2025

Do we have some higher level guidance we could give engineers for EuiErrorBoundary?

Like when should we consider using an ErrorBoundary? I think we can lean on how it's used in Kibana to provide guidance.

Is there typically a single, top-level ErrorBoundary? Or is this something in Kibana that people tend to use in a more targeted fashion? Per-page? Per-Plugin?

Should we use these when we're expecting error to occur, or as a general fail safe?

I don't think this needs a lot of thought or be totally comprehensive, but giving some sort of guidance here for Engineers would be helpful.

EDIT: Perhaps here we can just leverage the generic guidance for boundaries provided by React: https://react.dev/reference/react/Component#catching-rendering-errors-with-an-error-boundary

We could simply say:

This is an opinionated Error Boundary for EUI. If you're using EUI and you're going to implement an Error Boundary, we recommend using this for consistent UX. For more context and best practices for Error Boundaries, refer to the React docs.

@JasonStoltz JasonStoltz added documentation Issues or PRs that only affect documentation - will not need changelog entries feature request low hanging fruit An issue, often a bug, that is lower effort and clearly ought to be fixed labels Feb 28, 2025
@JasonStoltz
Copy link
Member Author

@clintandrewhall @tkajtoch Flagging this one as you may have some thoughts.

@clintandrewhall
Copy link
Contributor

EuiErrorBoundary has been fairly effective as a base component for catching React errors. Most of the extension of that has been to provide telemetry, (which obviously isn't much of an EUI concern, at least for now).

@tsullivan did a lot of work for Serverless with https://github.com/elastic/kibana-team/issues/646 That's a good reference for what we're doing with boundaries in Kibana (from a Shared UX/Platform perspective). Off the top of my head, we really need folks to wrap lazy components in boundaries, at a minimum.

I think there were some other explorations as to what we could "do" with Error Boundaries, once fired, from a user's perspective. They can certainly look nicer, but you get into a customization range that gets pretty hairy.

Some examples in the EUI docs would be great. Talking with @tsullivan might glean a bit of insight, as well as @Dosant wrt to the React 18 upgrade...?

@JasonStoltz
Copy link
Member Author

Pulling out a bit of relevant guidance from that linked issue:

For recoverable errors we should display inline the EUI error boundary component, this way the whole app is still functioning and usable, only the broken part is replaced by the error.

@JasonStoltz
Copy link
Member Author

FWIW, I didn't realize that Shared UX has KibanaErrorBoundary as well

Are there docs for that?

I gathered some context from this PR:

Kibana already has KibanaErrorBoundary component to catch thrown errors. Closer look at the component reveals it was designed to be applied at page level. The component doesn't accept any customization.

Obviously Kibana requires an error boundary component to catch thrown errors at section levels. Such error are usually fatal non-recoverable error happening due to unexpected data arrives from the storage. It may block critical workflows.

To mitigate workflow blocking and address section level errors a new KibanaSectionErrorBoundary component was added. It accepts sectionName property to properly reflect it in messages. On top of that it shared displaying error functionality with KibanaErrorBoundary.

I'm guessing there could be some confusion in Kibana about error boundaries, and when to use KibanaErrorBoundary vs KibanaSectionErrorBoundary vs EuiErrorBoundary. There are some style differences between the Shared UX offering and Shared UX offering as well.

It's outside the scope of this issue, so just thinking out loud. It could be nice to get the EUI Boundary to be consistent with the KibanaErrorBoundary. And/or if the EuiErrorBoundary is unnecessary, we could even consider deprecating it to reduce confusion in Kibana.

@JasonStoltz
Copy link
Member Author

@tsullivan Do you have any insight as to when we might want folks to use KibanaSectionErrorBoundary vs EuiErrorBoundary in Kibana?

@tsullivan
Copy link
Member

tsullivan commented Mar 3, 2025

@JasonStoltz My thoughts on error boundaries:

  1. Ideally end-users should never see an actual error boundary rendered. If the React component can throw errors during rendering, those should be caught internally and the component should render specific error messages, based on how the component functions, that help the user understand what happened and how to correct it. We can use other EUI components such as EuiCallout or toast messages to do that.
  2. Error boundaries should only be used to surround components at a high level, where we don't know how lower-level code works and we don't trust it to catch all of its own errors. Like @clintandrewhall wrote, wrapping lazy-loaded components in boundaries is a good practice.
  3. We try to preserve the integrity of server-side data by not allowing the user to make state changes using a UI with an active error state; for this reason, the error boundary should cover up UI controls that trigger updates to server-side data. Or the controls should be hidden some other way, like having the component be aware of when it's in an error state.
  4. KibanaSectionErrorBoundary is a new component, but it should be avoided. It breaks the ideals of points 1-3. It only exists because teams have asked for something that has the same look and feel as the main KibanaErrorBoundary, to use in places where it's hard catch errors at a low-level and show specific error messages.
  5. All error boundaries should have the same look and feel in Kibana, and for that reason it's not recommended to use EuiErrorBoundary at all. Its design does not match the one that was made more recently for KibanaErrorBoundary.

@JasonStoltz
Copy link
Member Author

Thanks Tim.

In that case, I think it's worth calling this out in our docs:

All error boundaries should have the same look and feel in Kibana, and for that reason it's not recommended to use EuiErrorBoundary at all. Its design does not match the one that was made more recently for KibanaErrorBoundary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues or PRs that only affect documentation - will not need changelog entries feature request low hanging fruit An issue, often a bug, that is lower effort and clearly ought to be fixed
Projects
None yet
Development

No branches or pull requests

3 participants