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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 35 additions & 4 deletions lib/Component.lua
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,10 @@ function Component:setState(mapState)
newState = assign({}, self.state, partialState)
end

if lifecyclePhase == ComponentLifecyclePhase.Init then
if lifecyclePhase == ComponentLifecyclePhase.Init or
lifecyclePhase == ComponentLifecyclePhase.ErrorBoundRecovery then
-- If `setState` is called in `init`, we can skip triggering an update!
-- This also happens when an error boundary is re-rendering itself.
local derivedState = self:__getDerivedState(self.props, newState)
self.state = assign(newState, derivedState)

Expand Down Expand Up @@ -186,6 +188,36 @@ function Component:render()
error(message, 0)
end

--[[
Tries to render a tree and update the virtual node, correctly handling the
presence of an error boundary.
]]
function Component:__updateVirtualNode(reconciler, virtualNode, renderResult)
local hostParent = virtualNode.hostParent
local instance = virtualNode.instance
local internalData = instance[InternalData]

-- Only bother with pcall if we actually have something to handle the error.
if self.getDerivedStateFromError ~= nil then
local success, message = pcall(reconciler.updateVirtualNodeWithRenderResult, virtualNode, hostParent, renderResult)

if not success then
local stateDelta = self.getDerivedStateFromError(message)
internalData.lifecyclePhase = ComponentLifecyclePhase.ErrorBoundRecovery
instance:setState(stateDelta)

internalData.lifecyclePhase = ComponentLifecyclePhase.Render
renderResult = instance:render()
internalData.lifecyclePhase = ComponentLifecyclePhase.ReconcileChildren
-- Don't try to handle errors at this point - the component should
-- be in a position to render a non-throwing fallback by now.
reconciler.updateVirtualNodeWithRenderResult(virtualNode, hostParent, renderResult)
end
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.


--[[
Performs property validation if the static method validateProps is declared.
validateProps should follow assert's expected arguments:
Expand Down Expand Up @@ -232,7 +264,6 @@ function Component:__mount(reconciler, virtualNode)
assert(Type.of(virtualNode) == Type.VirtualNode)

local currentElement = virtualNode.currentElement
local hostParent = virtualNode.hostParent

-- Contains all the information that we want to keep from consumers of
-- Roact, or even other parts of the codebase like the reconciler.
Expand Down Expand Up @@ -278,7 +309,7 @@ function Component:__mount(reconciler, virtualNode)
local renderResult = instance:render()

internalData.lifecyclePhase = ComponentLifecyclePhase.ReconcileChildren
reconciler.updateVirtualNodeWithRenderResult(virtualNode, hostParent, renderResult)
self:__updateVirtualNode(reconciler, virtualNode, renderResult)

if instance.didMount ~= nil then
internalData.lifecyclePhase = ComponentLifecyclePhase.DidMount
Expand Down Expand Up @@ -429,7 +460,7 @@ function Component:__resolveUpdate(incomingProps, incomingState)
local renderResult = virtualNode.instance:render()

internalData.lifecyclePhase = ComponentLifecyclePhase.ReconcileChildren
reconciler.updateVirtualNodeWithRenderResult(virtualNode, virtualNode.hostParent, renderResult)
self:__updateVirtualNode(reconciler, virtualNode, renderResult)

if self.didUpdate ~= nil then
internalData.lifecyclePhase = ComponentLifecyclePhase.DidUpdate
Expand Down
189 changes: 189 additions & 0 deletions lib/Component.spec/getDerivedStateFromError.spec.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,189 @@
return function()
local createElement = require(script.Parent.Parent.createElement)
local createReconciler = require(script.Parent.Parent.createReconciler)
local createSpy = require(script.Parent.Parent.createSpy)
local NoopRenderer = require(script.Parent.Parent.NoopRenderer)

local Component = require(script.Parent.Parent.Component)

local noopReconciler = createReconciler(NoopRenderer)

it("should not be called if the component doesn't throw", function()
local Boundary = Component:extend("Boundary")

local getDerivedSpy = createSpy(function(message)
return {}
end)

Boundary.getDerivedStateFromError = getDerivedSpy.value

function Boundary:render()
return nil
end

local element = createElement(Boundary)
local hostParent = nil
local key = "Test"

noopReconciler.mountVirtualNode(element, hostParent, key)
expect(getDerivedSpy.callCount).to.equal(0)
end)

it("should be called with the error message", function()
local function Bug()
error("test error")
end

local Boundary = Component:extend("Boundary")

local getDerivedSpy = createSpy(function(message)
-- The error message will not be the same as what's thrown because a
-- line/source object as well as stack trace will be attached to it
expect(message).to.be.ok()

return {}
end)

Boundary.getDerivedStateFromError = getDerivedSpy.value

function Boundary:render()
return createElement(Bug)
end

local element = createElement(Boundary)
local hostParent = nil
local key = "Test"

-- This will throw, because we don't stop rendering the throwing
-- component in response to the error. We test this more
-- rigorously elsewhere; this is just to keep the test from failing.
expect(function()
noopReconciler.mountVirtualNode(element, hostParent, key)
end).to.throw()
expect(getDerivedSpy.callCount).to.equal(1)
end)

it("should throw an error if the fallback render throws", function()
local function Bug()
error("test error")
end

local Boundary = Component:extend("Boundary")

function Boundary.getDerivedStateFromError(message)
return {}
end

local renderSpy = createSpy(function(self)
return createElement(Bug)
end)

Boundary.render = renderSpy.value

local element = createElement(Boundary)
local hostParent = nil
local key = "Test"

expect(function()
noopReconciler.mountVirtualNode(element, hostParent, key)
end).to.throw()
expect(renderSpy.callCount).to.equal(2)
end)

it("should return a state delta for the component", function()
local getStateCallback = nil

local function Bug()
error("test error")
end

local Boundary = Component:extend("Boundary")

function Boundary.getDerivedStateFromError(message)
return {
errored = true
}
end

function Boundary:init()
getStateCallback = function()
return self.state
end
end

local renderSpy
renderSpy = createSpy(function(self)
if renderSpy.callCount > 1 then
expect(self.state.errored).to.equal(true)
end

if self.state.errored then
return nil
else
return createElement(Bug)
end
end)

Boundary.render = renderSpy.value

local element = createElement(Boundary)
local hostParent = nil
local key = "Test"

noopReconciler.mountVirtualNode(element, hostParent, key)
expect(renderSpy.callCount).to.equal(2)
expect(getStateCallback().errored).to.equal(true)
end)

it("should not interrupt the lifecycle methods", function()
local setStateCallback = nil

local function Bug()
error("test error")
end

local Boundary = Component:extend("Boundary")

function Boundary.getDerivedStateFromError(message)
return {
errored = true
}
end

function Boundary:init()
setStateCallback = function(delta)
self:setState(delta)
end
end

function Boundary:render()
if self.state.errored then
return nil
else
return createElement(Bug)
end
end

local didMountSpy = createSpy()
Boundary.didMount = didMountSpy.value

local didUpdateSpy = createSpy()
Boundary.didUpdate = didUpdateSpy.value

local element = createElement(Boundary)
local hostParent = nil
local key = "Test"

noopReconciler.mountVirtualNode(element, hostParent, key)

expect(didMountSpy.callCount).to.equal(1)
expect(didUpdateSpy.callCount).to.equal(0)

setStateCallback({
errored = false
})

expect(didMountSpy.callCount).to.equal(1)
expect(didUpdateSpy.callCount).to.equal(1)
end)
end
1 change: 1 addition & 0 deletions lib/ComponentLifecyclePhase.lua
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ local ComponentLifecyclePhase = strict({
-- Phases describing reconciliation status
ReconcileChildren = Symbol.named("reconcileChildren"),
Idle = Symbol.named("idle"),
ErrorBoundRecovery = Symbol.named("errorBoundRecovery"),
}, "ComponentLifecyclePhase")

return ComponentLifecyclePhase