diff --git a/CHANGELOG.md b/CHANGELOG.md index 9eac0588..46b0613c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Roact Changelog ## Unreleased Changes +* Fixed a bug where the Roact tree could get into a broken state when using callbacks passed to a child component. Updated the tempFixUpdateChildrenReEntrancy config value to also handle this case. ([#315](https://github.com/Roblox/roact/pull/315)) * Fixed forwardRef description ([#312](https://github.com/Roblox/roact/pull/312)). ## [1.4.0](https://github.com/Roblox/roact/releases/tag/v1.4.0) (November 19th, 2020) diff --git a/rotriever.toml b/rotriever.toml index 3c39e3b5..15867ec9 100644 --- a/rotriever.toml +++ b/rotriever.toml @@ -3,4 +3,4 @@ name = "roblox/roact" author = "Roblox" license = "Apache-2.0" content_root = "src" -version = "1.4.0" \ No newline at end of file +version = "1.4.1" diff --git a/src/Component.lua b/src/Component.lua index 7c6b62a6..7e4d9186 100644 --- a/src/Component.lua +++ b/src/Component.lua @@ -157,22 +157,8 @@ function Component:setState(mapState) internalData.pendingState = assign(newState, derivedState) elseif lifecyclePhase == ComponentLifecyclePhase.Idle then - -- Pause parent events when we are updated outside of our lifecycle - -- If these events are not paused, our setState can cause a component higher up the - -- tree to rerender based on events caused by our component while this reconciliation is happening. - -- This could cause the tree to become invalid. - local virtualNode = internalData.virtualNode - local reconciler = internalData.reconciler - if config.tempFixUpdateChildrenReEntrancy then - reconciler.suspendParentEvents(virtualNode) - end - -- Outside of our lifecycle, the state update is safe to make immediately self:__update(nil, newState) - - if config.tempFixUpdateChildrenReEntrancy then - reconciler.resumeParentEvents(virtualNode) - end else local messageTemplate = invalidSetStateMessages.default diff --git a/src/RobloxRenderer.spec.lua b/src/RobloxRenderer.spec.lua index 9e8934ac..4735f78f 100644 --- a/src/RobloxRenderer.spec.lua +++ b/src/RobloxRenderer.spec.lua @@ -1025,5 +1025,335 @@ return function() reconciler.unmountVirtualNode(instance) end) end) + + it("should not allow re-entrancy in updateChildren even with callbacks", function() + local configValues = { + tempFixUpdateChildrenReEntrancy = true, + } + + GlobalConfig.scoped(configValues, function() + local LowestComponent = Component:extend("LowestComponent") + + function LowestComponent:render() + return createElement("Frame") + end + + function LowestComponent:didMount() + self.props.onDidMountCallback() + end + + local ChildComponent = Component:extend("ChildComponent") + + function ChildComponent:init() + self:setState({ + firstTime = true + }) + end + + local childCoroutine + + function ChildComponent:render() + if self.state.firstTime then + return createElement("Frame") + end + + return createElement(LowestComponent, { + onDidMountCallback = self.props.onDidMountCallback + }) + end + + function ChildComponent:didMount() + childCoroutine = coroutine.create(function() + self:setState({ + firstTime = false + }) + end) + end + + local ParentComponent = Component:extend("ParentComponent") + + local didMountCallbackCalled = 0 + + function ParentComponent:init() + self:setState({ + count = 1 + }) + + self.onDidMountCallback = function() + didMountCallbackCalled = didMountCallbackCalled + 1 + if self.state.count < 5 then + self:setState({ + count = self.state.count + 1, + }) + end + end + end + + function ParentComponent:render() + return createElement("Frame", { + + }, { + ChildComponent = createElement(ChildComponent, { + count = self.state.count, + onDidMountCallback = self.onDidMountCallback, + }) + }) + end + + local parent = Instance.new("ScreenGui") + parent.Parent = game.CoreGui + + local tree = createElement(ParentComponent) + + local hostKey = "Some Key" + local instance = reconciler.mountVirtualNode(tree, parent, hostKey) + + coroutine.resume(childCoroutine) + + expect(#parent:GetChildren()).to.equal(1) + + local frame = parent:GetChildren()[1] + + expect(#frame:GetChildren()).to.equal(1) + + -- In an ideal world, the didMount callback would probably be called only once. Since it is called by two different + -- LowestComponent instantiations 2 is also acceptable though. + expect(didMountCallbackCalled <= 2).to.equal(true) + + reconciler.unmountVirtualNode(instance) + end) + end) + + it("should never call unmount twice when tempFixUpdateChildrenReEntrancy is turned on", function() + local configValues = { + tempFixUpdateChildrenReEntrancy = true, + } + + GlobalConfig.scoped(configValues, function() + local unmountCounts = {} + + local function addUnmount(id) + unmountCounts[id] = unmountCounts[id] + 1 + end + + local function addInit(id) + unmountCounts[id] = 0 + end + + local LowestComponent = Component:extend("LowestComponent") + function LowestComponent:init() + addInit(tostring(self)) + end + + function LowestComponent:render() + return createElement("Frame") + end + + function LowestComponent:didMount() + self.props.onDidMountCallback() + end + + function LowestComponent:willUnmount() + addUnmount(tostring(self)) + end + + local FirstComponent = Component:extend("FirstComponent") + function FirstComponent:init() + addInit(tostring(self)) + end + + function FirstComponent:render() + return createElement("TextLabel") + end + + function FirstComponent:willUnmount() + addUnmount(tostring(self)) + end + + local ChildComponent = Component:extend("ChildComponent") + + function ChildComponent:init() + addInit(tostring(self)) + + self:setState({ + firstTime = true + }) + end + + local childCoroutine + + function ChildComponent:render() + if self.state.firstTime then + return createElement(FirstComponent) + end + + return createElement(LowestComponent, { + onDidMountCallback = self.props.onDidMountCallback + }) + end + + function ChildComponent:didMount() + childCoroutine = coroutine.create(function() + self:setState({ + firstTime = false + }) + end) + end + + function ChildComponent:willUnmount() + addUnmount(tostring(self)) + end + + local ParentComponent = Component:extend("ParentComponent") + + local didMountCallbackCalled = 0 + + function ParentComponent:init() + self:setState({ + count = 1 + }) + + self.onDidMountCallback = function() + didMountCallbackCalled = didMountCallbackCalled + 1 + if self.state.count < 5 then + self:setState({ + count = self.state.count + 1, + }) + end + end + end + + function ParentComponent:render() + return createElement("Frame", { + + }, { + ChildComponent = createElement(ChildComponent, { + count = self.state.count, + onDidMountCallback = self.onDidMountCallback, + }) + }) + end + + local parent = Instance.new("ScreenGui") + parent.Parent = game.CoreGui + + local tree = createElement(ParentComponent) + + local hostKey = "Some Key" + local instance = reconciler.mountVirtualNode(tree, parent, hostKey) + + coroutine.resume(childCoroutine) + + expect(#parent:GetChildren()).to.equal(1) + + local frame = parent:GetChildren()[1] + + expect(#frame:GetChildren()).to.equal(1) + + -- In an ideal world, the didMount callback would probably be called only once. Since it is called by two different + -- LowestComponent instantiations 2 is also acceptable though. + expect(didMountCallbackCalled <= 2).to.equal(true) + + reconciler.unmountVirtualNode(instance) + + for _, value in pairs(unmountCounts) do + expect(value).to.equal(1) + end + end) + end) + + it("should never unmount a node unnecesarily in the case of re-rentry", function() + local configValues = { + tempFixUpdateChildrenReEntrancy = true, + } + + GlobalConfig.scoped(configValues, function() + local LowestComponent = Component:extend("LowestComponent") + function LowestComponent:render() + return createElement("Frame") + end + + function LowestComponent:didUpdate(prevProps, prevState) + if prevProps.firstTime and not self.props.firstTime then + self.props.onChangedCallback() + end + end + + local ChildComponent = Component:extend("ChildComponent") + + function ChildComponent:init() + self:setState({ + firstTime = true + }) + end + + local childCoroutine + + function ChildComponent:render() + return createElement(LowestComponent, { + firstTime = self.state.firstTime, + onChangedCallback = self.props.onChangedCallback + }) + end + + function ChildComponent:didMount() + childCoroutine = coroutine.create(function() + self:setState({ + firstTime = false + }) + end) + end + + local ParentComponent = Component:extend("ParentComponent") + + local onChangedCallbackCalled = 0 + + function ParentComponent:init() + self:setState({ + count = 1 + }) + + self.onChangedCallback = function() + onChangedCallbackCalled = onChangedCallbackCalled + 1 + if self.state.count < 5 then + self:setState({ + count = self.state.count + 1, + }) + end + end + end + + function ParentComponent:render() + return createElement("Frame", { + + }, { + ChildComponent = createElement(ChildComponent, { + count = self.state.count, + onChangedCallback = self.onChangedCallback, + }) + }) + end + + local parent = Instance.new("ScreenGui") + parent.Parent = game.CoreGui + + local tree = createElement(ParentComponent) + + local hostKey = "Some Key" + local instance = reconciler.mountVirtualNode(tree, parent, hostKey) + + coroutine.resume(childCoroutine) + + expect(#parent:GetChildren()).to.equal(1) + + local frame = parent:GetChildren()[1] + + expect(#frame:GetChildren()).to.equal(1) + + expect(onChangedCallbackCalled).to.equal(1) + + reconciler.unmountVirtualNode(instance) + end) + end) end) -end \ No newline at end of file +end diff --git a/src/createReconciler.lua b/src/createReconciler.lua index 3ef3ee2f..2ff86467 100644 --- a/src/createReconciler.lua +++ b/src/createReconciler.lua @@ -45,7 +45,16 @@ local function createReconciler(renderer) local context = virtualNode.originalContext or virtualNode.context local parentLegacyContext = virtualNode.parentLegacyContext - unmountVirtualNode(virtualNode) + if config.tempFixUpdateChildrenReEntrancy then + -- If updating this node has caused a component higher up the tree to re-render + -- and updateChildren to be re-entered then this node could already have been + -- unmounted in the previous updateChildren pass. + if not virtualNode.wasUnmounted then + unmountVirtualNode(virtualNode) + end + else + unmountVirtualNode(virtualNode) + end local newNode = mountVirtualNode(newElement, hostParent, hostKey, context, parentLegacyContext) -- mountVirtualNode can return nil if the element is a boolean @@ -66,6 +75,10 @@ local function createReconciler(renderer) internalAssert(Type.of(virtualNode) == Type.VirtualNode, "Expected arg #1 to be of type VirtualNode") end + virtualNode.updateChildrenCount = virtualNode.updateChildrenCount + 1 + + local currentUpdateChildrenCount = virtualNode.updateChildrenCount + local removeKeys = {} -- Changed or removed children @@ -73,6 +86,18 @@ local function createReconciler(renderer) local newElement = ElementUtils.getElementByKey(newChildElements, childKey) local newNode = updateVirtualNode(childNode, newElement) + -- If updating this node has caused a component higher up the tree to re-render + -- and updateChildren to be re-entered for this virtualNode then + -- this result is invalid and needs to be disgarded. + if config.tempFixUpdateChildrenReEntrancy then + if virtualNode.updateChildrenCount ~= currentUpdateChildrenCount then + if newNode and newNode ~= virtualNode.children[childKey] then + unmountVirtualNode(newNode) + end + return + end + end + if newNode ~= nil then virtualNode.children[childKey] = newNode else @@ -100,6 +125,18 @@ local function createReconciler(renderer) virtualNode.legacyContext ) + -- If updating this node has caused a component higher up the tree to re-render + -- and updateChildren to be re-entered for this virtualNode then + -- this result is invalid and needs to be discarded. + if config.tempFixUpdateChildrenReEntrancy then + if virtualNode.updateChildrenCount ~= currentUpdateChildrenCount then + if childNode then + unmountVirtualNode(childNode) + end + return + end + end + -- mountVirtualNode can return nil if the element is a boolean if childNode ~= nil then childNode.depth = virtualNode.depth + 1 @@ -136,6 +173,8 @@ local function createReconciler(renderer) internalAssert(Type.of(virtualNode) == Type.VirtualNode, "Expected arg #1 to be of type VirtualNode") end + virtualNode.wasUnmounted = true + local kind = ElementKind.of(virtualNode.currentElement) if kind == ElementKind.Host then @@ -286,6 +325,8 @@ local function createReconciler(renderer) children = {}, hostParent = hostParent, hostKey = hostKey, + updateChildrenCount = 0, + wasUnmounted = false, -- Legacy Context API -- A table of context values inherited from the parent node @@ -441,28 +482,6 @@ local function createReconciler(renderer) return tree end - local function suspendParentEvents(virtualNode) - local parentNode = virtualNode.parent - while parentNode do - if parentNode.eventManager ~= nil then - parentNode.eventManager:suspend() - end - - parentNode = parentNode.parent - end - end - - local function resumeParentEvents(virtualNode) - local parentNode = virtualNode.parent - while parentNode do - if parentNode.eventManager ~= nil then - parentNode.eventManager:resume() - end - - parentNode = parentNode.parent - end - end - reconciler = { mountVirtualTree = mountVirtualTree, unmountVirtualTree = unmountVirtualTree, @@ -474,9 +493,6 @@ local function createReconciler(renderer) updateVirtualNode = updateVirtualNode, updateVirtualNodeWithChildren = updateVirtualNodeWithChildren, updateVirtualNodeWithRenderResult = updateVirtualNodeWithRenderResult, - - suspendParentEvents = suspendParentEvents, - resumeParentEvents = resumeParentEvents, } return reconciler