Further update children re-rentrancy problems (#315)

This fixes the problem where nodes lower down the Roact tree 
can change outside of their standard lifecycle and then cause a 
callback or event to a node higher up the tree. That component
can re-render while the node further down the tree is updating 
its children, causing duplicates of elements to be added.

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)

rotriever.toml

@@ -3,4 +3,4 @@ name = "roblox/roact"
 author = "Roblox"
 license = "Apache-2.0"
 content_root = "src"
-version = "1.4.0"
+version = "1.4.1"

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
 

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
+end