diff --git a/lib/Component.lua b/lib/Component.lua index 31bf7930..b3f870c7 100644 --- a/lib/Component.lua +++ b/lib/Component.lua @@ -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) @@ -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 + --[[ Performs property validation if the static method validateProps is declared. validateProps should follow assert's expected arguments: @@ -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. @@ -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 @@ -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 diff --git a/lib/Component.spec/getDerivedStateFromError.spec.lua b/lib/Component.spec/getDerivedStateFromError.spec.lua new file mode 100644 index 00000000..d0957623 --- /dev/null +++ b/lib/Component.spec/getDerivedStateFromError.spec.lua @@ -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 \ No newline at end of file diff --git a/lib/ComponentLifecyclePhase.lua b/lib/ComponentLifecyclePhase.lua index dd239635..a2de7118 100644 --- a/lib/ComponentLifecyclePhase.lua +++ b/lib/ComponentLifecyclePhase.lua @@ -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 \ No newline at end of file diff --git a/modules/lemur b/modules/lemur index 9b72fddd..fa8c2959 160000 --- a/modules/lemur +++ b/modules/lemur @@ -1 +1 @@ -Subproject commit 9b72fddd39dc2ad590b800138ec56421f4fbc6d1 +Subproject commit fa8c2959dacbe9972f0e42ba4be8ae422fe71a3d