Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Error boundaries! #173

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

AmaranthineCodices
Copy link
Contributor

Adds error boundaries like React's newer version of them. An error boundary is any component that declares a method with the signature static getDerivedStateFromError(message: string) -> setStateMapper. If this method is declared, the component will capture any errors from its children, pass the message to getDerivedStateFromError, apply the resulting state delta, then re-try the render.

The boundary will re-try the render only once; if the re-tried render throws, that error will propagate upwards until it either surfaces or hits another error boundary. It is the responsibility of the error boundary component to make sure this doesn't happen. If the re-render succeeds, the normal lifecycle process (calling either didMount or didUpdate depending on which is appropriate) will continue.

Questions I'm not sure about:

  • Is this actually safe, or does this secretly corrupt some state that causes problems later?
  • Should getDerivedStateFromError be allowed to return a mapper function, or should it only be allowed to return a dictionary? Allowing a function to be returned gives the function, indirectly, access to the component's state and props tables.

Checklist before submitting:

  • Added/updated relevant tests

Copy link
Contributor

@ZoteTheMighty ZoteTheMighty left a comment

Choose a reason for hiding this comment

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

This looks good! I appreciate the thorough tests.

I think it's probably fine to allow returning a stateUpdater, mostly because it should make it easier to respond to errors intelligently. Just getting the error message doesn't help much if you want to branch on different kinds of errors because it backs you into doing string matching. Maybe being able to optionally see the props/state helps component authors respond more intelligently?

That said, the typical pattern is to create dedicated ErrorBoundary components that know what to do with errors, so that component's own props/state may or may not be of much value.

lib/Component.spec/getDerivedStateFromError.spec.lua Outdated Show resolved Hide resolved
lib/Component.lua Outdated Show resolved Hide resolved
Copy link
Contributor

@ZoteTheMighty ZoteTheMighty left a comment

Choose a reason for hiding this comment

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

Whoops, realized I'd started this comment but conversed about it offline before posting it. Leaving the comment so it's clear why we haven't come back to this change yet.

else
reconciler.updateVirtualNodeWithRenderResult(virtualNode, hostParent, renderResult)
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

It just occurred to me that this implementation doesn't actually catch errors if they occur downstream of this call site.
For example, suppose I have:

ComponentA
|--ErrorBound
|----ComponentB

Consider these scenarios:

  1. ComponentA renders, causing ComponentB to render. ComponentB throws an error. The ErrorBound catches and handles it just fine.
  2. After all of this is mounted, ComponentB has a state update triggered, and re-renders. ComponentB throws an error. The ErrorBound won't be involved in reconciling it, so it won't be invoked!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I need to revisit this when I have the time - there are definitely cases that error boundaries should catch, but don't.

@AmaranthineCodices AmaranthineCodices changed the base branch from new-reconciler to master May 6, 2019 17:57
@LPGhatguy LPGhatguy added the status: needs design Used when we're not sure what the best way forward is label Nov 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: needs design Used when we're not sure what the best way forward is
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants