Make Fragment an Element

CHANGELOG.md

@@ -1,6 +1,7 @@
 # Roact Changelog
 
 ## Unreleased Changes
+* Fixed a bug where fragments could not be used as children of an element or another fragment. ([#214](https://github.com/Roblox/roact/pull/214))
 
 ## [1.1.0](https://github.com/Roblox/roact/releases/tag/v1.1.0) (June 3rd, 2019)
 * 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))
@@ -52,4 +53,4 @@ This release significantly reworks Roact internals to enable new features and op
 * Got rid of installer scripts in favor of regular model files
 
 ## December 1, 2017 Prerelease
-* Initial pre-release build
+* Initial pre-release build

src/ElementKind.lua

@@ -19,6 +19,7 @@ local ElementKindInternal = {
 	Host = Symbol.named("Host"),
 	Function = Symbol.named("Function"),
 	Stateful = Symbol.named("Stateful"),
+	Fragment = Symbol.named("Fragment"),
 }
 
 function ElementKindInternal.of(value)

src/ElementUtils.lua

@@ -40,10 +40,6 @@ ElementUtils.UseParentKey = Symbol.named("UseParentKey")
 function ElementUtils.iterateElements(elementOrElements)
 	local richType = Type.of(elementOrElements)
 
-	if richType == Type.Fragment then
-		return pairs(elementOrElements.elements)
-	end
-
 	-- Single child
 	if richType == Type.Element then
 		local called = false
@@ -93,10 +89,6 @@ function ElementUtils.getElementByKey(elements, hostKey)
 		return nil
 	end
 
-	if Type.of(elements) == Type.Fragment then
-		return elements.elements[hostKey]
-	end
-
 	if typeof(elements) == "table" then
 		return elements[hostKey]
 	end

src/ElementUtils.spec.lua

@@ -17,27 +17,6 @@ return function()
 			expect(iteratedKey).to.equal(nil)
 		end)
 
-		it("should iterate over fragments", function()
-			local children = createFragment({
-				a = createElement("TextLabel"),
-				b = createElement("TextLabel"),
-			})
-
-			local seenChildren = {}
-			local count = 0
-
-			for key, child in ElementUtils.iterateElements(children) do
-				expect(typeof(key)).to.equal("string")
-				expect(Type.of(child)).to.equal(Type.Element)
-				seenChildren[child] = key
-				count = count + 1
-			end
-
-			expect(count).to.equal(2)
-			expect(seenChildren[children.elements.a]).to.equal("a")
-			expect(seenChildren[children.elements.b]).to.equal("b")
-		end)
-
 		it("should iterate over tables", function()
 			local children = {
 				a = createElement("TextLabel"),
@@ -97,16 +76,6 @@ return function()
 			end)
 		end)
 
-		it("should return the corresponding element from a fragment", function()
-			local children = createFragment({
-				a = createElement("TextLabel"),
-				b = createElement("TextLabel"),
-			})
-
-			expect(ElementUtils.getElementByKey(children, "a")).to.equal(children.elements.a)
-			expect(ElementUtils.getElementByKey(children, "b")).to.equal(children.elements.b)
-		end)
-
 		it("should return the corresponding element from a table", function()
 			local children = {
 				a = createElement("TextLabel"),

src/RobloxRenderer.spec.lua

@@ -4,6 +4,7 @@ return function()
 	local Children = require(script.Parent.PropMarkers.Children)
 	local Component = require(script.Parent.Component)
 	local createElement = require(script.Parent.createElement)
+	local createFragment = require(script.Parent.createFragment)
 	local createReconciler = require(script.Parent.createReconciler)
 	local createRef = require(script.Parent.createRef)
 	local createSpy = require(script.Parent.createSpy)
@@ -686,6 +687,124 @@ return function()
 		end)
 	end)
 
+	describe("Fragments", function()
+		it("should parent the fragment's elements into the fragment's parent", function()
+			local hostParent = Instance.new("Folder")
+
+			local fragment = createFragment({
+				key = createElement("IntValue", {
+					Value = 1,
+				}),
+				key2 = createElement("IntValue", {
+					Value = 2,
+				}),
+			})
+
+			local node = reconciler.mountVirtualNode(fragment, hostParent, "test")
+
+			expect(hostParent:FindFirstChild("key")).to.be.ok()
+			expect(hostParent.key.ClassName).to.equal("IntValue")
+			expect(hostParent.key.Value).to.equal(1)
+
+			expect(hostParent:FindFirstChild("key2")).to.be.ok()
+			expect(hostParent.key2.ClassName).to.equal("IntValue")
+			expect(hostParent.key2.Value).to.equal(2)
+
+			reconciler.unmountVirtualNode(node)
+
+			expect(#hostParent:GetChildren()).to.equal(0)
+		end)
+
+		it("should allow sibling fragment to have common keys", function()
+			local hostParent = Instance.new("Folder")
+			local hostKey = "Test"
+
+			local function parent(props)
+				return createElement("IntValue", {}, {
+					fragmentA = createFragment({
+						key = createElement("StringValue", {
+							Value = "A",
+						}),
+						key2 = createElement("StringValue", {
+							Value = "B",
+						}),
+					}),
+					fragmentB = createFragment({
+						key = createElement("StringValue", {
+							Value = "C",
+						}),
+						key2 = createElement("StringValue", {
+							Value = "D",
+						}),
+					}),
+				})
+			end
+
+			local node = reconciler.mountVirtualNode(createElement(parent), hostParent, hostKey)
+			local parentChildren = hostParent[hostKey]:GetChildren()
+
+			expect(#parentChildren).to.equal(4)
+
+			local childValues = {}
+
+			for _, child in pairs(parentChildren) do
+				expect(child.ClassName).to.equal("StringValue")
+				childValues[child.Value] = 1 + (childValues[child.Value] or 0)
+			end
+
+			-- check if the StringValues have not collided
+			expect(childValues.A).to.equal(1)
+			expect(childValues.B).to.equal(1)
+			expect(childValues.C).to.equal(1)
+			expect(childValues.D).to.equal(1)
+
+			reconciler.unmountVirtualNode(node)
+
+			expect(#hostParent:GetChildren()).to.equal(0)
+		end)
+
+		it("should render nested fragments", function()
+			local hostParent = Instance.new("Folder")
+
+			local fragment = createFragment({
+				key = createFragment({
+					TheValue = createElement("IntValue", {
+						Value = 1,
+					}),
+					TheOtherValue = createElement("IntValue", {
+						Value = 2,
+					})
+				})
+			})
+
+			local node = reconciler.mountVirtualNode(fragment, hostParent, "Test")
+
+			expect(hostParent:FindFirstChild("TheValue")).to.be.ok()
+			expect(hostParent.TheValue.ClassName).to.equal("IntValue")
+			expect(hostParent.TheValue.Value).to.equal(1)
+
+			expect(hostParent:FindFirstChild("TheOtherValue")).to.be.ok()
+			expect(hostParent.TheOtherValue.ClassName).to.equal("IntValue")
+			expect(hostParent.TheOtherValue.Value).to.equal(2)
+
+			reconciler.unmountVirtualNode(node)
+
+			expect(#hostParent:GetChildren()).to.equal(0)
+		end)
+
+		it("should not add any instances if the fragment is empty", function()
+			local hostParent = Instance.new("Folder")
+
+			local node = reconciler.mountVirtualNode(createFragment({}), hostParent, "test")
+
+			expect(#hostParent:GetChildren()).to.equal(0)
+
+			reconciler.unmountVirtualNode(node)
+
+			expect(#hostParent:GetChildren()).to.equal(0)
+		end)
+	end)
+
 	describe("Context", function()
 		it("should pass context values through Roblox host nodes", function()
 			local Consumer = Component:extend("Consumer")

src/Type.lua

@@ -22,7 +22,6 @@ end
 
 addType("Binding")
 addType("Element")
-addType("Fragment")
 addType("HostChangeEvent")
 addType("HostEvent")
 addType("StatefulComponentClass")

src/createFragment.lua

@@ -1,8 +1,10 @@
+local ElementKind = require(script.Parent.ElementKind)
 local Type = require(script.Parent.Type)
 
 local function createFragment(elements)
 	return {
-		[Type] = Type.Fragment,
+		[Type] = Type.Element,
+		[ElementKind] = ElementKind.Fragment,
 		elements = elements,
 	}
 end

src/createFragment.spec.lua

@@ -0,0 +1,21 @@
+return function()
+	local ElementKind = require(script.Parent.ElementKind)
+	local Type = require(script.Parent.Type)
+
+	local createFragment = require(script.Parent.createFragment)
+
+	it("should create new primitive elements", function()
+		local fragment = createFragment({})
+
+		expect(fragment).to.be.ok()
+		expect(Type.of(fragment)).to.equal(Type.Element)
+		expect(ElementKind.of(fragment)).to.equal(ElementKind.Fragment)
+	end)
+
+	it("should accept children", function()
+		local subFragment = createFragment({})
+		local fragment = createFragment({key = subFragment})
+
+		expect(fragment.elements.key).to.equal(subFragment)
+	end)
+end

src/createReconciler.lua

@@ -138,6 +138,10 @@ local function createReconciler(renderer)
 			for _, childNode in pairs(virtualNode.children) do
 				unmountVirtualNode(childNode)
 			end
+		elseif kind == ElementKind.Fragment then
+			for _, childNode in pairs(virtualNode.children) do
+				unmountVirtualNode(childNode)
+			end
 		else
 			error(("Unknown ElementKind %q"):format(tostring(kind), 2))
 		end
@@ -170,6 +174,12 @@ local function createReconciler(renderer)
 		return virtualNode
 	end
 
+	local function updateFragmentVirtualNode(virtualNode, newElement)
+		updateVirtualNodeWithChildren(virtualNode, virtualNode.hostParent, newElement.elements)
+
+		return virtualNode
+	end
+
 	--[[
 		Update the given virtual node using a new element describing what it
 		should transform into.
@@ -219,6 +229,8 @@ local function createReconciler(renderer)
 			shouldContinueUpdate = virtualNode.instance:__update(newElement, newState)
 		elseif kind == ElementKind.Portal then
 			virtualNode = updatePortalVirtualNode(virtualNode, newElement)
+		elseif kind == ElementKind.Fragment then
+			virtualNode = updateFragmentVirtualNode(virtualNode, newElement)
 		else
 			error(("Unknown ElementKind %q"):format(tostring(kind), 2))
 		end
@@ -283,6 +295,13 @@ local function createReconciler(renderer)
 		updateVirtualNodeWithChildren(virtualNode, targetHostParent, children)
 	end
 
+	local function mountFragmentVirtualNode(virtualNode)
+		local element = virtualNode.currentElement
+		local children = element.elements
+
+		updateVirtualNodeWithChildren(virtualNode, virtualNode.hostParent, children)
+	end
+
 	--[[
 		Constructs a new virtual node and mounts it, but does not place it into
 		the tree.
@@ -317,6 +336,8 @@ local function createReconciler(renderer)
 			element.component:__mount(reconciler, virtualNode)
 		elseif kind == ElementKind.Portal then
 			mountPortalVirtualNode(virtualNode)
+		elseif kind == ElementKind.Fragment then
+			mountFragmentVirtualNode(virtualNode)
 		else
 			error(("Unknown ElementKind %q"):format(tostring(kind), 2))
 		end

src/createReconciler.spec.lua

@@ -5,6 +5,7 @@ return function()
 	local createSpy = require(script.Parent.createSpy)
 	local NoopRenderer = require(script.Parent.NoopRenderer)
 	local Type = require(script.Parent.Type)
+	local ElementKind = require(script.Parent.ElementKind)
 
 	local createReconciler = require(script.Parent.createReconciler)
 
@@ -47,7 +48,7 @@ return function()
 		end)
 	end)
 
-	describe("elements and fragments", function()
+	describe("invalid elements", function()
 		it("should throw errors when attempting to mount invalid elements", function()
 			-- These function components return values with incorrect types
 			local returnsString = function()
@@ -286,4 +287,40 @@ return function()
 			expect(childBComponentSpy.callCount).to.equal(1)
 		end)
 	end)
+
+	describe("Fragments", function()
+		it("should mount fragments", function()
+			local fragment = createFragment({})
+			local node = noopReconciler.mountVirtualNode(fragment, nil, "test")
+
+			expect(node).to.be.ok()
+			expect(ElementKind.of(node.currentElement)).to.equal(ElementKind.Fragment)
+		end)
+
+		it("should mount an empty fragment", function()
+			local emptyFragment = createFragment({})
+			local node = noopReconciler.mountVirtualNode(emptyFragment, nil, "test")
+
+			expect(node).to.be.ok()
+			expect(next(node.children)).to.never.be.ok()
+		end)
+
+		it("should mount all fragment's children", function()
+			local childComponentSpy = createSpy(function(props)
+				return nil
+			end)
+			local elements = {}
+			local totalElements = 5
+
+			for i=1, totalElements do
+				elements["key"..tostring(i)] = createElement(childComponentSpy.value, {})
+			end
+
+			local fragments = createFragment(elements)
+			local node = noopReconciler.mountVirtualNode(fragments, nil, "test")
+
+			expect(node).to.be.ok()
+			expect(childComponentSpy.callCount).to.equal(totalElements)
+		end)
+	end)
 end