Fix cases where properties don't correctly reconcile to nil (#148)

lib/Reconciler.lua

@@ -459,17 +459,6 @@ function Reconciler._reconcilePrimitiveProps(fromElement, toElement, rbx)
 
 		local newValue = toElement.props[key]
 
-		-- Assume any property that can be set to nil has a default value of nil
-		if newValue == nil then
-			local _, value = getDefaultPropertyValue(rbx.ClassName, key)
-
-			-- We don't care if getDefaultPropertyValue fails, because
-			-- _setRbxProp will catch the error below.
-			newValue = value
-		end
-
-		-- Roblox does this check for normal values, but we have special
-		-- properties like events that warrant this.
 		if oldValue ~= newValue then
 			Reconciler._setRbxProp(rbx, key, newValue, toElement)
 		end
@@ -478,8 +467,6 @@ function Reconciler._reconcilePrimitiveProps(fromElement, toElement, rbx)
 	-- Set properties that are new in toElement
 	for key, newValue in pairs(toElement.props) do
 		if not seenProps[key] then
-			seenProps[key] = true
-
 			local oldValue = fromElement.props[key]
 
 			if oldValue ~= newValue then
@@ -511,6 +498,19 @@ function Reconciler._setRbxProp(rbx, key, value, element)
 	if type(key) == "string" then
 		-- Regular property
 
+		if value == nil then
+			-- Here, we assume that any properties for which nil is a valid
+			-- value have nil as their default value. This is necessary because
+			-- we can't distinguish between a property being set to nil and not
+			-- being present at all in a table; we assume the latter.
+
+			local hasProperty, defaultValue = getDefaultPropertyValue(rbx.ClassName, key)
+
+			if hasProperty then
+				value = defaultValue
+			end
+		end
+
 		local success, err = pcall(set, rbx, key, value)
 
 		if not success then
@@ -529,9 +529,17 @@ function Reconciler._setRbxProp(rbx, key, value, element)
 		-- Special property with extra data attached.
 
 		if key.type == Event then
-			Reconciler._singleEventManager:connect(rbx, key.name, value)
+			if value ~= nil then
+				Reconciler._singleEventManager:connect(rbx, key.name, value)
+			else
+				Reconciler._singleEventManager:disconnect(rbx, key.name)
+			end
 		elseif key.type == Change then
-			Reconciler._singleEventManager:connectProperty(rbx, key.name, value)
+			if value ~= nil then
+				Reconciler._singleEventManager:connectProperty(rbx, key.name, value)
+			else
+				Reconciler._singleEventManager:disconnectProperty(rbx, key.name)
+			end
 		else
 			local source = element.source or DEFAULT_SOURCE
 

lib/Reconciler.spec.lua

@@ -1,5 +1,7 @@
 return function()
 	local Core = require(script.Parent.Core)
+	local Event = require(script.Parent.Event)
+	local Change = require(script.Parent.Change)
 	local createRef = require(script.Parent.createRef)
 	local createElement = require(script.Parent.createElement)
 
@@ -85,4 +87,64 @@ return function()
 		Reconciler.unmount(handle)
 		expect(bRef.current).to.never.be.ok()
 	end)
+
+	it("should clean up Event references properly", function()
+		local sizeChangedCount = 0
+
+		local function eventCallback(object, property)
+			if property == "Size" then
+				sizeChangedCount = sizeChangedCount + 1
+			end
+		end
+
+		local element = createElement("Frame", {
+			[Event.Changed] = eventCallback,
+		})
+
+		local container = Instance.new("Folder")
+		local handle = Reconciler.mount(element, container, "Foo")
+		expect(handle).to.be.ok()
+		expect(sizeChangedCount).to.equal(0)
+		expect(container.Foo).to.be.ok()
+
+		container.Foo.Size = UDim2.new(0, 100, 0, 100)
+		expect(sizeChangedCount).to.equal(1)
+
+		handle = Reconciler.reconcile(handle, createElement("Frame", {
+			[Event.Changed] = nil
+		}))
+		container.Foo.Size = UDim2.new(0, 200, 0, 200)
+		expect(sizeChangedCount).to.equal(1)
+
+		Reconciler.unmount(handle)
+	end)
+
+	it("should clean up Change references properly", function()
+		local sizeChangedCount = 0
+
+		local function changeCallback()
+			sizeChangedCount = sizeChangedCount + 1
+		end
+
+		local element = createElement("Frame", {
+			[Change.Size] = changeCallback,
+		})
+
+		local container = Instance.new("Folder")
+		local handle = Reconciler.mount(element, container, "Foo")
+		expect(handle).to.be.ok()
+		expect(sizeChangedCount).to.equal(0)
+		expect(container.Foo).to.be.ok()
+
+		container.Foo.Size = UDim2.new(0, 100, 0, 100)
+		expect(sizeChangedCount).to.equal(1)
+
+		handle = Reconciler.reconcile(handle, createElement("Frame", {
+			[Change.Size] = nil
+		}))
+		container.Foo.Size = UDim2.new(0, 200, 0, 200)
+		expect(sizeChangedCount).to.equal(1)
+
+		Reconciler.unmount(handle)
+	end)
 end