LUAFDN-268 optimize createSignal memory usage

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## Unreleased Changes
 * 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))
 
 ## [1.3.1](https://github.com/Roblox/roact/releases/tag/v1.3.0) (November 19th, 2020)
 * Added component name to property validation error message ([#275](https://github.com/Roblox/roact/pull/275))

src/createSignal.lua

@@ -12,58 +12,51 @@
 		disconnect()
 ]]
 
-local function addToMap(map, addKey, addValue)
-	local new = {}
-
-	for key, value in pairs(map) do
-		new[key] = value
-	end
-
-	new[addKey] = addValue
-
-	return new
-end
-
-local function removeFromMap(map, removeKey)
-	local new = {}
-
-	for key, value in pairs(map) do
-		if key ~= removeKey then
-			new[key] = value
-		end
-	end
-
-	return new
-end
-
 local function createSignal()
 	local connections = {}
+	local suspendedConnections = {}
+	local firing = false
 
 	local function subscribe(self, callback)
 		assert(typeof(callback) == "function", "Can only subscribe to signals with a function.")
 
 		local connection = {
 			callback = callback,
+			disconnected = false,
 		}
 
-		connections = addToMap(connections, callback, connection)
+		-- If the callback is already registered, don't add to the suspendedConnection. Otherwise, this will disable
+		-- the existing one.
+		if firing and not connections[callback] then
+			suspendedConnections[callback] = connection
+		end
+
+		connections[callback] = connection
 
 		local function disconnect()
 			assert(not connection.disconnected, "Listeners can only be disconnected once.")
 
 			connection.disconnected = true
-			connections = removeFromMap(connections, callback)
+			connections[callback] = nil
+			suspendedConnections[callback] = nil
 		end
 
 		return disconnect
 	end
 
 	local function fire(self, ...)
+		firing = true
 		for callback, connection in pairs(connections) do
-			if not connection.disconnected then
+			if not connection.disconnected and not suspendedConnections[callback] then
 				callback(...)
 			end
 		end
+
+		firing = false
+
+		for callback, _ in pairs(suspendedConnections) do
+			suspendedConnections[callback] = nil
+		end
 	end
 
 	return {
@@ -72,4 +65,4 @@ local function createSignal()
 	}
 end
 
-return createSignal
+return createSignal

src/createSignal.spec.lua

@@ -86,4 +86,69 @@ return function()
 		-- Exactly once listener should have been called.
 		expect(spyA.callCount + spyB.callCount).to.equal(1)
 	end)
+
+	it("should allow adding listener in the middle of firing", function()
+		local signal = createSignal()
+
+		local disconnectA
+		local spyA = createSpy()
+		local listener = function(a, b)
+			disconnectA = signal:subscribe(spyA.value)
+		end
+
+		local disconnectListener = signal:subscribe(listener)
+
+		expect(spyA.callCount).to.equal(0)
+
+		local a = {}
+		local b = 67
+		signal:fire(a, b)
+
+		expect(spyA.callCount).to.equal(0)
+
+		-- The new listener should be picked up in next fire.
+		signal:fire(b, a)
+		expect(spyA.callCount).to.equal(1)
+		spyA:assertCalledWith(b, a)
+
+		disconnectA()
+		disconnectListener()
+
+		signal:fire(a)
+
+		expect(spyA.callCount).to.equal(1)
+	end)
+
+	it("should have one connection instance when add the same listener multiple times", function()
+		local signal = createSignal()
+
+		local spyA = createSpy()
+		local disconnect1 = signal:subscribe(spyA.value)
+
+		expect(spyA.callCount).to.equal(0)
+
+		local a = {}
+		local b = 67
+		signal:fire(a, b)
+
+		expect(spyA.callCount).to.equal(1)
+		spyA:assertCalledWith(a, b)
+
+		local disconnect2 = signal:subscribe(spyA.value)
+
+		signal:fire(b, a)
+		expect(spyA.callCount).to.equal(2)
+		spyA:assertCalledWith(b, a)
+
+		disconnect2()
+
+		signal:fire(a)
+
+		expect(spyA.callCount).to.equal(2)
+
+		-- should have no effect.
+		disconnect1()
+		signal:fire(a)
+		expect(spyA.callCount).to.equal(2)
+	end)
 end