From f6c1dbd152a1d776c514553c4d18861088abe2ff Mon Sep 17 00:00:00 2001 From: Lily Brown Date: Thu, 23 May 2019 15:53:12 -0700 Subject: [PATCH 1/6] Revert performance improvement This caused an issue when updating an element that had children - when an element went from having children to having nil children, the rendered objects would not be removed. --- src/RobloxRenderer.lua | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/RobloxRenderer.lua b/src/RobloxRenderer.lua index 5cd67cf9..afedccd9 100644 --- a/src/RobloxRenderer.lua +++ b/src/RobloxRenderer.lua @@ -211,9 +211,7 @@ function RobloxRenderer.mountHostNode(reconciler, virtualNode) local children = element.props[Children] - if children ~= nil then - reconciler.updateVirtualNodeWithChildren(virtualNode, virtualNode.hostObject, children) - end + reconciler.updateVirtualNodeWithChildren(virtualNode, virtualNode.hostObject, children) instance.Parent = hostParent virtualNode.hostObject = instance @@ -268,10 +266,7 @@ function RobloxRenderer.updateHostNode(reconciler, virtualNode, newElement) error(fullMessage, 0) end - local children = newElement.props[Children] - if children ~= nil then - reconciler.updateVirtualNodeWithChildren(virtualNode, virtualNode.hostObject, children) - end + reconciler.updateVirtualNodeWithChildren(virtualNode, virtualNode.hostObject, newElement.props[Children]) if virtualNode.eventManager ~= nil then virtualNode.eventManager:resume() From 752ca4e2ca659a7bbbf64f91079a4a00fdbe85a3 Mon Sep 17 00:00:00 2001 From: Lily Brown Date: Thu, 23 May 2019 15:57:45 -0700 Subject: [PATCH 2/6] add test to verify that #209 is fixed --- src/RobloxRenderer.spec.lua | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/RobloxRenderer.spec.lua b/src/RobloxRenderer.spec.lua index 2b3f36e4..1da3161c 100644 --- a/src/RobloxRenderer.spec.lua +++ b/src/RobloxRenderer.spec.lua @@ -419,6 +419,33 @@ return function() expect(message:find("RobloxRenderer%.spec")).to.be.ok() end) end) + + it("should delete instances when reconciling to nil children", function() + local parent = Instance.new("Folder") + local key = "Some Key" + + local element = createElement("Frame", { + Size = UDim2.new(1, 0, 1, 0), + }, { + child = createElement("Frame"), + }) + + local node = reconciler.createVirtualNode(element, parent, key) + + RobloxRenderer.mountHostNode(reconciler, node) + + expect(#parent:GetChildren()).to.equal(1) + + local instance = parent:GetChildren()[1] + expect(#instance:GetChildren()).to.equal(1) + + local newElement = createElement("Frame", { + Size = UDim2.new(0.5, 0, 0.5, 0), + }) + + RobloxRenderer.updateHostNode(reconciler, node, newElement) + expect(#instance:GetChildren()).to.equal(0) + end) end) describe("unmountHostNode", function() From 6032e88d1c75b10945f1e33424a1a39b3c48c98a Mon Sep 17 00:00:00 2001 From: Lily Brown Date: Thu, 23 May 2019 16:05:06 -0700 Subject: [PATCH 3/6] update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14f8518d..329c6bb9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Roact Changelog +## Unreleased + +* Fixed an issue where updating a host element with children to an element with `nil` children caused the old children to not be unmounted. + ## [1.0.0](https://github.com/Roblox/roact/releases/tag/v1.0.0) This release significantly reworks Roact internals to enable new features and optimizations. From 179f8b9ffe2a2e35f5060000a823dfe90fd4e5d8 Mon Sep 17 00:00:00 2001 From: Paul Doyle Date: Thu, 30 May 2019 09:34:48 -0700 Subject: [PATCH 4/6] Dial back bugfix. Reintroduce performance consideration, add additional check to resolve the bug. --- src/RobloxRenderer.lua | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/RobloxRenderer.lua b/src/RobloxRenderer.lua index afedccd9..d19079be 100644 --- a/src/RobloxRenderer.lua +++ b/src/RobloxRenderer.lua @@ -211,7 +211,9 @@ function RobloxRenderer.mountHostNode(reconciler, virtualNode) local children = element.props[Children] - reconciler.updateVirtualNodeWithChildren(virtualNode, virtualNode.hostObject, children) + if children ~= nil then + reconciler.updateVirtualNodeWithChildren(virtualNode, virtualNode.hostObject, children) + end instance.Parent = hostParent virtualNode.hostObject = instance @@ -266,7 +268,11 @@ function RobloxRenderer.updateHostNode(reconciler, virtualNode, newElement) error(fullMessage, 0) end - reconciler.updateVirtualNodeWithChildren(virtualNode, virtualNode.hostObject, newElement.props[Children]) + local children = newElement.props[Children] + + if children ~= nil or oldProps[Children] ~= nil then + reconciler.updateVirtualNodeWithChildren(virtualNode, virtualNode.hostObject, newElement.props[Children]) + end if virtualNode.eventManager ~= nil then virtualNode.eventManager:resume() From 9bb91059c1e1fedc4801e845097e37935e74fa96 Mon Sep 17 00:00:00 2001 From: Paul Doyle Date: Thu, 30 May 2019 11:32:35 -0700 Subject: [PATCH 5/6] Preempt review comments --- CHANGELOG.md | 2 +- src/RobloxRenderer.lua | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 329c6bb9..04061efe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased -* Fixed an issue where updating a host element with children to an element with `nil` children caused the old children to not be unmounted. +* Fixed an issue where updating a host element with children to an element with `nil` children caused the old children to not be unmounted. ([#210](https://github.com/Roblox/roact/pull/210)) ## [1.0.0](https://github.com/Roblox/roact/releases/tag/v1.0.0) This release significantly reworks Roact internals to enable new features and optimizations. diff --git a/src/RobloxRenderer.lua b/src/RobloxRenderer.lua index d19079be..d2c7b79b 100644 --- a/src/RobloxRenderer.lua +++ b/src/RobloxRenderer.lua @@ -271,7 +271,7 @@ function RobloxRenderer.updateHostNode(reconciler, virtualNode, newElement) local children = newElement.props[Children] if children ~= nil or oldProps[Children] ~= nil then - reconciler.updateVirtualNodeWithChildren(virtualNode, virtualNode.hostObject, newElement.props[Children]) + reconciler.updateVirtualNodeWithChildren(virtualNode, virtualNode.hostObject, children) end if virtualNode.eventManager ~= nil then From 97a77ea8d4cb4df5263bf5e24bfca2889cfcd528 Mon Sep 17 00:00:00 2001 From: Lily <31936135+AmaranthineCodices@users.noreply.github.com> Date: Thu, 30 May 2019 12:00:18 -0700 Subject: [PATCH 6/6] Update src/RobloxRenderer.lua Co-Authored-By: Lucien Greathouse --- src/RobloxRenderer.lua | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/RobloxRenderer.lua b/src/RobloxRenderer.lua index d2c7b79b..4f528ad5 100644 --- a/src/RobloxRenderer.lua +++ b/src/RobloxRenderer.lua @@ -269,7 +269,6 @@ function RobloxRenderer.updateHostNode(reconciler, virtualNode, newElement) end local children = newElement.props[Children] - if children ~= nil or oldProps[Children] ~= nil then reconciler.updateVirtualNodeWithChildren(virtualNode, virtualNode.hostObject, children) end @@ -281,4 +280,4 @@ function RobloxRenderer.updateHostNode(reconciler, virtualNode, newElement) return virtualNode end -return RobloxRenderer \ No newline at end of file +return RobloxRenderer