diff --git a/CHANGELOG.md b/CHANGELOG.md index d79ca119..16fe6dcc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ # Roact Changelog ## Unreleased Changes +* Fixed a bug where the Roact tree could get into a broken state when processing changes to child instances outside the standard lifecycle. + This change is behind the config value tempFixUpdateChildrenReEntrancy ([#301](https://github.com/Roblox/roact/pull/301)) * Added color schemes for documentation based on user preference ([#290](https://github.com/Roblox/roact/pull/290)). * Fixed stack trace level when throwing an error in `createReconciler` ([#297](https://github.com/Roblox/roact/pull/297)). * Optimized the memory usage of 'createSignal' implementation. ([#304](https://github.com/Roblox/roact/pull/304)) diff --git a/src/Component.lua b/src/Component.lua index 12833745..7c6b62a6 100644 --- a/src/Component.lua +++ b/src/Component.lua @@ -157,9 +157,22 @@ 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/Config.lua b/src/Config.lua index 6884bc47..54043503 100644 --- a/src/Config.lua +++ b/src/Config.lua @@ -22,6 +22,10 @@ local defaultConfig = { ["elementTracing"] = false, -- Enables validation of component props in stateful components. ["propValidation"] = false, + + -- Temporary config for enabling a bug fix for processing events based on updates to child instances + -- outside of the standard lifecycle. + ["tempFixUpdateChildrenReEntrancy"] = false, } -- Build a list of valid configuration values up for debug messages. diff --git a/src/RobloxRenderer.spec.lua b/src/RobloxRenderer.spec.lua index 1ea04cb2..9e8934ac 100644 --- a/src/RobloxRenderer.spec.lua +++ b/src/RobloxRenderer.spec.lua @@ -11,6 +11,7 @@ return function() local GlobalConfig = require(script.Parent.GlobalConfig) local Portal = require(script.Parent.Portal) local Ref = require(script.Parent.PropMarkers.Ref) + local Event = require(script.Parent.PropMarkers.Event) local RobloxRenderer = require(script.Parent.RobloxRenderer) @@ -946,4 +947,83 @@ return function() }) end) end) + + + describe("Integration Tests", function() + it("should not allow re-entrancy in updateChildren", function() + local configValues = { + tempFixUpdateChildrenReEntrancy = true, + } + + GlobalConfig.scoped(configValues, function() + 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("TextLabel") + end + + function ChildComponent:didMount() + childCoroutine = coroutine.create(function() + self:setState({ + firstTime = false + }) + end) + end + + local ParentComponent = Component:extend("ParentComponent") + + function ParentComponent:init() + self:setState({ + count = 1 + }) + + self.childAdded = function() + self:setState({ + count = self.state.count + 1, + }) + end + end + + function ParentComponent:render() + return createElement("Frame", { + [Event.ChildAdded] = self.childAdded, + }, { + ChildComponent = createElement(ChildComponent, { + count = self.state.count + }) + }) + 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) + + reconciler.unmountVirtualNode(instance) + end) + end) + end) end \ No newline at end of file diff --git a/src/createReconciler.lua b/src/createReconciler.lua index 928b9832..3ef3ee2f 100644 --- a/src/createReconciler.lua +++ b/src/createReconciler.lua @@ -37,6 +37,7 @@ local function createReconciler(renderer) local hostParent = virtualNode.hostParent local hostKey = virtualNode.hostKey local depth = virtualNode.depth + local parent = virtualNode.parent -- If the node that is being replaced has modified context, we need to -- use the original *unmodified* context for the new node @@ -50,6 +51,7 @@ local function createReconciler(renderer) -- mountVirtualNode can return nil if the element is a boolean if newNode ~= nil then newNode.depth = depth + newNode.parent = parent end return newNode @@ -101,6 +103,7 @@ local function createReconciler(renderer) -- mountVirtualNode can return nil if the element is a boolean if childNode ~= nil then childNode.depth = virtualNode.depth + 1 + childNode.parent = virtualNode virtualNode.children[childKey] = childNode end end @@ -279,6 +282,7 @@ local function createReconciler(renderer) [Type] = Type.VirtualNode, currentElement = element, depth = 1, + parent = nil, children = {}, hostParent = hostParent, hostKey = hostKey, @@ -437,6 +441,28 @@ 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, @@ -448,6 +474,9 @@ local function createReconciler(renderer) updateVirtualNode = updateVirtualNode, updateVirtualNodeWithChildren = updateVirtualNodeWithChildren, updateVirtualNodeWithRenderResult = updateVirtualNodeWithRenderResult, + + suspendParentEvents = suspendParentEvents, + resumeParentEvents = resumeParentEvents, } return reconciler