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

feat(react): add ErrorBoundary component #4619

Merged

Conversation

joshblack
Copy link
Contributor

React introduced error boundary handling in v16. This PR aims to form an opinion on what an error boundary means for a design system. At a high-level, this works by doing the following:

<ErrorBoundary fallback={<Fallback />}>
  <ComponentThatMayThrow />
</ErrorBoundary>

With this setup, if ComponentThatMayThrow or any of its children have an error the component passed to fallback will be displayed instead.

This API was heavily inspired by Data fetching with Suspense in Relay

image

The way this may work in product is as follows:

  • Identify the root-level error boundary UI you would like to display if everything goes wrong
  • Starting from the root, find the highest sections/regions in your project and create boundaries for them.
    • In particular, focus on sections that may have data dependencies

In addition, ErrorBoundary has an optional ErrorBoundaryContext that may be supplied. The only method currently is log which is called in componentDidCatch as a side-effect. This may be useful for logging errors to an external service.

Changelog

New

  • Add ErrorBoundary and ErrorBoundaryContext

Changed

Removed

@joshblack joshblack requested a review from a team as a code owner November 7, 2019 22:28
@ghost ghost requested a review from aledavila November 7, 2019 22:28
@netlify
Copy link

netlify bot commented Nov 7, 2019

Deploy preview for carbon-elements ready!

Built with commit c910ea4

https://deploy-preview-4619--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Nov 7, 2019

Deploy preview for carbon-components-react ready!

Built with commit c910ea4

https://deploy-preview-4619--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Nov 7, 2019

Deploy preview for the-carbon-components ready!

Built with commit c910ea4

https://deploy-preview-4619--the-carbon-components.netlify.com

@joshblack
Copy link
Contributor Author

@jendowns any tips on getting the prop table to generate correctly? 😂

Copy link
Contributor

@jendowns jendowns left a comment

Choose a reason for hiding this comment

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

Hey @joshblack! I have an idea for this --


return 'Successfully rendered';
}

Copy link
Contributor

@jendowns jendowns Nov 8, 2019

Choose a reason for hiding this comment

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

@joshblack You could add this to give DemoComponent the same info as ErrorBoundary, so it will show the right component name in the story source + also show the props in the prop table.

(Though unfortunately all props will be node type -- I'm not sure how to fix that yet Oh oops, they ARE type node 😆 Disregard this comment then)

Suggested change
DemoComponent.displayName = 'ErrorBoundary';
DemoComponent.__docgenInfo = ErrorBoundary.__docgenInfo;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be a dumb question, but what's the difference between this proposal and:

DemoComponent.__docgenInfo = ErrorBoundary.__docgenInfo;

Just noticed the double spread and was curious.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh there's no difference! I think I've personally gotten into the habit of doing this as a way to very explicitly show "I'm need the props here" etc 😆 I'll update my suggestions to be more succinct 👍


return 'Successfully rendered';
}

Copy link
Contributor

@jendowns jendowns Nov 8, 2019

Choose a reason for hiding this comment

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

Same as above:

Suggested change
DemoComponent.displayName = 'ErrorBoundary';
DemoComponent.__docgenInfo = ErrorBoundary.__docgenInfo;

@joshblack
Copy link
Contributor Author

@aledavila let me know if you want to go through this today!

@joshblack
Copy link
Contributor Author

Same to you @vpicone if you want to chat about it 😄

Copy link
Contributor

@vpicone vpicone left a comment

Choose a reason for hiding this comment

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

@joshblack looks solid. Is this meant as a helper for us internally? There's nothing Carbon specific with how this would be implemented right?

@joshblack
Copy link
Contributor Author

@vpicone nah, just a pattern that seems to come up more that would be great to bring in so folks could use it. Just makes it easier to build stuff with Carbon 😄

@joshblack
Copy link
Contributor Author

Let me know if you have any questions @vpicone @aledavila!

Copy link
Contributor

@aledavila aledavila left a comment

Choose a reason for hiding this comment

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

Looks great

@joshblack
Copy link
Contributor Author

Let me know if you have any questions @vpicone!

@vpicone
Copy link
Contributor

vpicone commented Nov 16, 2019

@joshblack no questions, just not sure I agree that it goes with the rest of the library. I don’t think we should include components that have nothing to do with the design language but increase or surface area for no reason other than someone might find it helpful.

What if react-router/gatsby/next started exporting an ErrorBoundary helper. If I’m an app dev do I use there’s or Carbons? Both/Either? I think increasing the surface area of the library and the types of the components we export might not actually make Carbon easier to build with.

@joshblack
Copy link
Contributor Author

@vpicone happy to chat about this in person, I think this matches our path forward with exposing best practices and patterns for building UIs at IBM. I think as a system when we start going into patterns territory we're not so much targeting a design language as helping folks build better product experiences. I think this component helps further that goal and would be a great addition.

Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

Looks good to me. And I agree that helpers like this (and a few others we've talked about) will be more and more necessary when we're shipping whole experiences via patterns :hurtrealbad:

* consumers can specify their own logic for logging errors. For example,
* reporting an error in the UI to an external service for every `ErrorBoundary`
* used.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

lovely documentation.

@joshblack joshblack requested a review from a team as a code owner November 20, 2019 22:57
@ghost ghost requested review from asudoh and emyarod November 20, 2019 22:58
@joshblack joshblack merged commit 1c1bff8 into carbon-design-system:master Nov 20, 2019
@joshblack joshblack deleted the feat/add-error-boundary branch November 20, 2019 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants