From 6ed4b8c2859df5deed32c7d8261b311630ded58c Mon Sep 17 00:00:00 2001 From: Conor Griffin Date: Fri, 1 Oct 2021 10:48:05 -0700 Subject: [PATCH] Remove tempFixUpdateChildrenReEntrancy (#324) We have had this turned on in a few projects for the last few months and it has proven sound. Remove tempFixUpdateChildrenReEntrancy so it is fixed by default. --- src/Config.lua | 4 - src/RobloxRenderer.spec.lua | 542 +++++++++++++++++------------------- src/createReconciler.lua | 32 +-- 3 files changed, 271 insertions(+), 307 deletions(-) diff --git a/src/Config.lua b/src/Config.lua index 0c73a25e..d97c8dce 100644 --- a/src/Config.lua +++ b/src/Config.lua @@ -22,10 +22,6 @@ 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 5473f1de..19be63c0 100644 --- a/src/RobloxRenderer.spec.lua +++ b/src/RobloxRenderer.spec.lua @@ -961,403 +961,379 @@ return function() end) it("should not allow re-entrancy in updateChildren", function() - local configValues = { - tempFixUpdateChildrenReEntrancy = true, - } - - GlobalConfig.scoped(configValues, function() - local ChildComponent = Component:extend("ChildComponent") + 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 + function ChildComponent:init() + self:setState({ + firstTime = true, + }) + end - return createElement("TextLabel") - end + local childCoroutine - function ChildComponent:didMount() - childCoroutine = coroutine.create(function() - self:setState({ - firstTime = false, - }) - end) + function ChildComponent:render() + if self.state.firstTime then + return createElement("Frame") end - local ParentComponent = Component:extend("ParentComponent") + return createElement("TextLabel") + end - function ParentComponent:init() + function ChildComponent:didMount() + childCoroutine = coroutine.create(function() self:setState({ - count = 1, + firstTime = false, }) + end) + end - self.childAdded = function() - self:setState({ - count = self.state.count + 1, - }) - end - end + local ParentComponent = Component:extend("ParentComponent") - function ParentComponent:render() - return createElement("Frame", { - [Event.ChildAdded] = self.childAdded, - }, { - ChildComponent = createElement(ChildComponent, { - count = self.state.count, - }), + function ParentComponent:init() + self:setState({ + count = 1, + }) + + self.childAdded = function() + self:setState({ + count = self.state.count + 1, }) end + end - local parent = Instance.new("ScreenGui") - parent.Parent = temporaryParent + function ParentComponent:render() + return createElement("Frame", { + [Event.ChildAdded] = self.childAdded, + }, { + ChildComponent = createElement(ChildComponent, { + count = self.state.count, + }), + }) + end - local tree = createElement(ParentComponent) + local parent = Instance.new("ScreenGui") + parent.Parent = temporaryParent - local hostKey = "Some Key" - local instance = reconciler.mountVirtualNode(tree, parent, hostKey) + local tree = createElement(ParentComponent) - coroutine.resume(childCoroutine) + local hostKey = "Some Key" + local instance = reconciler.mountVirtualNode(tree, parent, hostKey) - expect(#parent:GetChildren()).to.equal(1) + coroutine.resume(childCoroutine) - local frame = parent:GetChildren()[1] + expect(#parent:GetChildren()).to.equal(1) - expect(#frame:GetChildren()).to.equal(1) + local frame = parent:GetChildren()[1] - reconciler.unmountVirtualNode(instance) - end) + expect(#frame:GetChildren()).to.equal(1) + + reconciler.unmountVirtualNode(instance) end) it("should not allow re-entrancy in updateChildren even with callbacks", function() - local configValues = { - tempFixUpdateChildrenReEntrancy = true, - } + local LowestComponent = Component:extend("LowestComponent") - GlobalConfig.scoped(configValues, function() - local LowestComponent = Component:extend("LowestComponent") + function LowestComponent:render() + return createElement("Frame") + end - function LowestComponent:render() - return createElement("Frame") - end + function LowestComponent:didMount() + self.props.onDidMountCallback() + end - function LowestComponent:didMount() - self.props.onDidMountCallback() + 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 - local ChildComponent = Component:extend("ChildComponent") + return createElement(LowestComponent, { + onDidMountCallback = self.props.onDidMountCallback, + }) + end - function ChildComponent:init() + function ChildComponent:didMount() + childCoroutine = coroutine.create(function() self:setState({ - firstTime = true, + firstTime = false, }) - end + end) + end - local childCoroutine + local ParentComponent = Component:extend("ParentComponent") - function ChildComponent:render() - if self.state.firstTime then - return createElement("Frame") - end + local didMountCallbackCalled = 0 - return createElement(LowestComponent, { - onDidMountCallback = self.props.onDidMountCallback, - }) - end + function ParentComponent:init() + self:setState({ + count = 1, + }) - function ChildComponent:didMount() - childCoroutine = coroutine.create(function() + self.onDidMountCallback = function() + didMountCallbackCalled = didMountCallbackCalled + 1 + if self.state.count < 5 then self:setState({ - firstTime = false, + count = self.state.count + 1, }) - end) + end end + end - local ParentComponent = Component:extend("ParentComponent") + function ParentComponent:render() + return createElement("Frame", {}, { + ChildComponent = createElement(ChildComponent, { + count = self.state.count, + onDidMountCallback = self.onDidMountCallback, + }), + }) + end - local didMountCallbackCalled = 0 + local parent = Instance.new("ScreenGui") + parent.Parent = temporaryParent - function ParentComponent:init() - self:setState({ - count = 1, - }) + local tree = createElement(ParentComponent) - self.onDidMountCallback = function() - didMountCallbackCalled = didMountCallbackCalled + 1 - if self.state.count < 5 then - self:setState({ - count = self.state.count + 1, - }) - end - end - end + local hostKey = "Some Key" + local instance = reconciler.mountVirtualNode(tree, parent, hostKey) - function ParentComponent:render() - return createElement("Frame", {}, { - ChildComponent = createElement(ChildComponent, { - count = self.state.count, - onDidMountCallback = self.onDidMountCallback, - }), - }) - end + coroutine.resume(childCoroutine) - local parent = Instance.new("ScreenGui") - parent.Parent = temporaryParent + expect(#parent:GetChildren()).to.equal(1) - local tree = createElement(ParentComponent) + local frame = parent:GetChildren()[1] - local hostKey = "Some Key" - local instance = reconciler.mountVirtualNode(tree, parent, hostKey) + expect(#frame:GetChildren()).to.equal(1) - coroutine.resume(childCoroutine) + -- 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) - expect(#parent:GetChildren()).to.equal(1) + reconciler.unmountVirtualNode(instance) + end) - local frame = parent:GetChildren()[1] + it("should never call unmount twice in the case of update children re-rentrancy", function() + local unmountCounts = {} - expect(#frame:GetChildren()).to.equal(1) + local function addUnmount(id) + unmountCounts[id] = unmountCounts[id] + 1 + end - -- 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) + local function addInit(id) + unmountCounts[id] = 0 + end - reconciler.unmountVirtualNode(instance) - end) - end) + local LowestComponent = Component:extend("LowestComponent") + function LowestComponent:init() + addInit(tostring(self)) + end - it("should never call unmount twice when tempFixUpdateChildrenReEntrancy is turned on", function() - local configValues = { - tempFixUpdateChildrenReEntrancy = true, - } + function LowestComponent:render() + return createElement("Frame") + end - GlobalConfig.scoped(configValues, function() - local unmountCounts = {} + function LowestComponent:didMount() + self.props.onDidMountCallback() + end - local function addUnmount(id) - unmountCounts[id] = unmountCounts[id] + 1 - end + function LowestComponent:willUnmount() + addUnmount(tostring(self)) + end - local function addInit(id) - unmountCounts[id] = 0 - end + local FirstComponent = Component:extend("FirstComponent") + function FirstComponent:init() + addInit(tostring(self)) + end - local LowestComponent = Component:extend("LowestComponent") - function LowestComponent:init() - addInit(tostring(self)) - end + function FirstComponent:render() + return createElement("TextLabel") + end - function LowestComponent:render() - return createElement("Frame") - end + function FirstComponent:willUnmount() + addUnmount(tostring(self)) + end - function LowestComponent:didMount() - self.props.onDidMountCallback() - end + local ChildComponent = Component:extend("ChildComponent") - function LowestComponent:willUnmount() - addUnmount(tostring(self)) - end + function ChildComponent:init() + addInit(tostring(self)) - local FirstComponent = Component:extend("FirstComponent") - function FirstComponent:init() - addInit(tostring(self)) - end + self:setState({ + firstTime = true, + }) + end - function FirstComponent:render() - return createElement("TextLabel") - end + local childCoroutine - function FirstComponent:willUnmount() - addUnmount(tostring(self)) + function ChildComponent:render() + if self.state.firstTime then + return createElement(FirstComponent) end - local ChildComponent = Component:extend("ChildComponent") - - function ChildComponent:init() - addInit(tostring(self)) + return createElement(LowestComponent, { + onDidMountCallback = self.props.onDidMountCallback, + }) + end + function ChildComponent:didMount() + childCoroutine = coroutine.create(function() self:setState({ - firstTime = true, + firstTime = false, }) - end + end) + end - local childCoroutine + function ChildComponent:willUnmount() + addUnmount(tostring(self)) + end - function ChildComponent:render() - if self.state.firstTime then - return createElement(FirstComponent) - end + local ParentComponent = Component:extend("ParentComponent") - return createElement(LowestComponent, { - onDidMountCallback = self.props.onDidMountCallback, - }) - end + local didMountCallbackCalled = 0 + + function ParentComponent:init() + self:setState({ + count = 1, + }) - function ChildComponent:didMount() - childCoroutine = coroutine.create(function() + self.onDidMountCallback = function() + didMountCallbackCalled = didMountCallbackCalled + 1 + if self.state.count < 5 then self:setState({ - firstTime = false, + count = self.state.count + 1, }) - 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 + end - function ParentComponent:render() - return createElement("Frame", {}, { - ChildComponent = createElement(ChildComponent, { - count = self.state.count, - onDidMountCallback = self.onDidMountCallback, - }), - }) - 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 = temporaryParent + local parent = Instance.new("ScreenGui") + parent.Parent = temporaryParent - local tree = createElement(ParentComponent) + local tree = createElement(ParentComponent) - local hostKey = "Some Key" - local instance = reconciler.mountVirtualNode(tree, parent, hostKey) + local hostKey = "Some Key" + local instance = reconciler.mountVirtualNode(tree, parent, hostKey) - coroutine.resume(childCoroutine) + coroutine.resume(childCoroutine) - expect(#parent:GetChildren()).to.equal(1) + expect(#parent:GetChildren()).to.equal(1) - local frame = parent:GetChildren()[1] + local frame = parent:GetChildren()[1] - expect(#frame:GetChildren()).to.equal(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) + -- 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) + reconciler.unmountVirtualNode(instance) - for _, value in pairs(unmountCounts) do - expect(value).to.equal(1) - end - end) + for _, value in pairs(unmountCounts) do + expect(value).to.equal(1) + end end) it("should never unmount a node unnecesarily in the case of re-rentry", function() - local configValues = { - tempFixUpdateChildrenReEntrancy = true, - } + local LowestComponent = Component:extend("LowestComponent") + function LowestComponent:render() + return createElement("Frame") + end - GlobalConfig.scoped(configValues, function() - local LowestComponent = Component:extend("LowestComponent") - function LowestComponent:render() - return createElement("Frame") + function LowestComponent:didUpdate(prevProps, _prevState) + if prevProps.firstTime and not self.props.firstTime then + self.props.onChangedCallback() end + 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") - local ChildComponent = Component:extend("ChildComponent") + function ChildComponent:init() + self:setState({ + firstTime = true, + }) + end - function ChildComponent:init() - self:setState({ - firstTime = true, - }) - end + local childCoroutine - local childCoroutine + function ChildComponent:render() + return createElement(LowestComponent, { + firstTime = self.state.firstTime, + onChangedCallback = self.props.onChangedCallback, + }) + end - function ChildComponent:render() - return createElement(LowestComponent, { - firstTime = self.state.firstTime, - onChangedCallback = self.props.onChangedCallback, + function ChildComponent:didMount() + childCoroutine = coroutine.create(function() + self:setState({ + firstTime = false, }) - end - - function ChildComponent:didMount() - childCoroutine = coroutine.create(function() - self:setState({ - firstTime = false, - }) - end) - end + end) + end - local ParentComponent = Component:extend("ParentComponent") + local ParentComponent = Component:extend("ParentComponent") - local onChangedCallbackCalled = 0 + local onChangedCallbackCalled = 0 - function ParentComponent:init() - self:setState({ - count = 1, - }) + 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 + 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 + function ParentComponent:render() + return createElement("Frame", {}, { + ChildComponent = createElement(ChildComponent, { + count = self.state.count, + onChangedCallback = self.onChangedCallback, + }), + }) + end - local parent = Instance.new("ScreenGui") - parent.Parent = temporaryParent + local parent = Instance.new("ScreenGui") + parent.Parent = temporaryParent - local tree = createElement(ParentComponent) + local tree = createElement(ParentComponent) - local hostKey = "Some Key" - local instance = reconciler.mountVirtualNode(tree, parent, hostKey) + local hostKey = "Some Key" + local instance = reconciler.mountVirtualNode(tree, parent, hostKey) - coroutine.resume(childCoroutine) + coroutine.resume(childCoroutine) - expect(#parent:GetChildren()).to.equal(1) + expect(#parent:GetChildren()).to.equal(1) - local frame = parent:GetChildren()[1] + local frame = parent:GetChildren()[1] - expect(#frame:GetChildren()).to.equal(1) + expect(#frame:GetChildren()).to.equal(1) - expect(onChangedCallbackCalled).to.equal(1) + expect(onChangedCallbackCalled).to.equal(1) - reconciler.unmountVirtualNode(instance) - end) + reconciler.unmountVirtualNode(instance) end) end) end diff --git a/src/createReconciler.lua b/src/createReconciler.lua index 0d7046fa..92e22346 100644 --- a/src/createReconciler.lua +++ b/src/createReconciler.lua @@ -46,14 +46,10 @@ local function createReconciler(renderer) local context = virtualNode.originalContext or virtualNode.context local parentLegacyContext = virtualNode.parentLegacyContext - 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 + -- 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 local newNode = mountVirtualNode(newElement, hostParent, hostKey, context, parentLegacyContext) @@ -90,13 +86,11 @@ local function createReconciler(renderer) -- 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 + if virtualNode.updateChildrenCount ~= currentUpdateChildrenCount then + if newNode and newNode ~= virtualNode.children[childKey] then + unmountVirtualNode(newNode) end + return end if newNode ~= nil then @@ -129,13 +123,11 @@ local function createReconciler(renderer) -- 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 + if virtualNode.updateChildrenCount ~= currentUpdateChildrenCount then + if childNode then + unmountVirtualNode(childNode) end + return end -- mountVirtualNode can return nil if the element is a boolean