Fixes issues with refs in the plugin. (#1005)

.gitignore

@@ -19,3 +19,7 @@
 
 # Snapshot files from the 'insta' Rust crate
 **/*.snap.new
+
+# Macos file system junk
+._*
+.DS_STORE

plugin/src/Reconciler/applyPatch.lua

@@ -16,6 +16,7 @@ local invariant = require(script.Parent.Parent.invariant)
 
 local decodeValue = require(script.Parent.decodeValue)
 local reify = require(script.Parent.reify)
+local reifyInstance, applyDeferredRefs = reify.reifyInstance, reify.applyDeferredRefs
 local setProperty = require(script.Parent.setProperty)
 
 local function applyPatch(instanceMap, patch)
@@ -29,6 +30,11 @@ local function applyPatch(instanceMap, patch)
 	-- Tracks any portions of the patch that could not be applied to the DOM.
 	local unappliedPatch = PatchSet.newEmpty()
 
+	-- Contains a list of all of the ref properties that we'll need to assign.
+	-- It is imperative that refs are assigned after all instances are created
+	-- to ensure that referents can be mapped to instances correctly.
+	local deferredRefs = {}
+
 	for _, removedIdOrInstance in ipairs(patch.removed) do
 		local removeInstanceSuccess = pcall(function()
 			if Types.RbxId(removedIdOrInstance) then
@@ -78,7 +84,7 @@ local function applyPatch(instanceMap, patch)
 			)
 		end
 
-		local failedToReify = reify(instanceMap, patch.added, id, parentInstance)
+		local failedToReify = reifyInstance(deferredRefs, instanceMap, patch.added, id, parentInstance)
 
 		if not PatchSet.isEmpty(failedToReify) then
 			Log.debug("Failed to reify as part of applying a patch: {:#?}", failedToReify)
@@ -143,7 +149,7 @@ local function applyPatch(instanceMap, patch)
 				[update.id] = mockVirtualInstance,
 			}
 
-			local failedToReify = reify(instanceMap, mockAdded, update.id, instance.Parent)
+			local failedToReify = reifyInstance(deferredRefs, instanceMap, mockAdded, update.id, instance.Parent)
 
 			local newInstance = instanceMap.fromIds[update.id]
 
@@ -206,6 +212,18 @@ local function applyPatch(instanceMap, patch)
 
 		if update.changedProperties ~= nil then
 			for propertyName, propertyValue in pairs(update.changedProperties) do
+				-- Because refs may refer to instances that we haven't constructed yet,
+				-- we defer applying any ref properties until all instances are created.
+				if next(propertyValue) == "Ref" then
+					table.insert(deferredRefs, {
+						id = update.id,
+						instance = instance,
+						propertyName = propertyName,
+						virtualValue = propertyValue,
+					})
+					continue
+				end
+
 				local decodeSuccess, decodedValue = decodeValue(propertyValue, instanceMap)
 				if not decodeSuccess then
 					unappliedUpdate.changedProperties[propertyName] = propertyValue
@@ -230,6 +248,8 @@ local function applyPatch(instanceMap, patch)
 		ChangeHistoryService:FinishRecording(historyRecording, Enum.FinishRecordingOperation.Commit)
 	end
 
+	applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch)
+
 	return unappliedPatch
 end
 

plugin/src/Reconciler/diff.lua

@@ -151,7 +151,24 @@ local function diff(instanceMap, virtualInstances, rootId)
 
 			if getProperySuccess then
 				local existingValue = existingValueOrErr
-				local decodeSuccess, decodedValue = decodeValue(virtualValue, instanceMap)
+				local decodeSuccess, decodedValue
+
+				-- If `virtualValue` is a ref then instead of decoding it to an instance,
+				-- we change `existingValue` to be a ref. This is because `virtualValue`
+				-- may point to an Instance which doesn't exist yet and therefore
+				-- decoding it may throw an error.
+				if next(virtualValue) == "Ref" then
+					decodeSuccess, decodedValue = true, virtualValue
+
+					if existingValue and typeof(existingValue) == "Instance" then
+						local existingValueRef = instanceMap.fromInstances[existingValue]
+						if existingValueRef then
+							existingValue = { Ref = existingValueRef }
+						end
+					end
+				else
+					decodeSuccess, decodedValue = decodeValue(virtualValue, instanceMap)
+				end
 
 				if decodeSuccess then
 					if not trueEquals(existingValue, decodedValue) then

plugin/src/Reconciler/reify.lua

@@ -7,26 +7,6 @@ local PatchSet = require(script.Parent.Parent.PatchSet)
 local setProperty = require(script.Parent.setProperty)
 local decodeValue = require(script.Parent.decodeValue)
 
-local reifyInner, applyDeferredRefs
-
-local function reify(instanceMap, virtualInstances, rootId, parentInstance)
-	-- Create an empty patch that will be populated with any parts of this reify
-	-- that could not happen, like instances that couldn't be created and
-	-- properties that could not be assigned.
-	local unappliedPatch = PatchSet.newEmpty()
-
-	-- Contains a list of all of the ref properties that we'll need to assign
-	-- after all instances are created. We apply refs in a second pass, after
-	-- we create as many instances as we can, so that we ensure that referents
-	-- can be mapped to instances correctly.
-	local deferredRefs = {}
-
-	reifyInner(instanceMap, virtualInstances, rootId, parentInstance, unappliedPatch, deferredRefs)
-	applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch)
-
-	return unappliedPatch
-end
-
 --[[
 	Add the given ID and all of its descendants in virtualInstances to the given
 	PatchSet, marked for addition.
@@ -40,10 +20,21 @@ local function addAllToPatch(patchSet, virtualInstances, id)
 	end
 end
 
+function reifyInstance(deferredRefs, instanceMap, virtualInstances, rootId, parentInstance)
+	-- Create an empty patch that will be populated with any parts of this reify
+	-- that could not happen, like instances that couldn't be created and
+	-- properties that could not be assigned.
+	local unappliedPatch = PatchSet.newEmpty()
+
+	reifyInstanceInner(unappliedPatch, deferredRefs, instanceMap, virtualInstances, rootId, parentInstance)
+
+	return unappliedPatch
+end
+
 --[[
 	Inner function that defines the core routine.
 ]]
-function reifyInner(instanceMap, virtualInstances, id, parentInstance, unappliedPatch, deferredRefs)
+function reifyInstanceInner(unappliedPatch, deferredRefs, instanceMap, virtualInstances, id, parentInstance)
 	local virtualInstance = virtualInstances[id]
 
 	if virtualInstance == nil then
@@ -102,7 +93,7 @@ function reifyInner(instanceMap, virtualInstances, id, parentInstance, unapplied
 	end
 
 	for _, childId in ipairs(virtualInstance.Children) do
-		reifyInner(instanceMap, virtualInstances, childId, instance, unappliedPatch, deferredRefs)
+		reifyInstanceInner(unappliedPatch, deferredRefs, instanceMap, virtualInstances, childId, instance)
 	end
 
 	instance.Parent = parentInstance
@@ -143,6 +134,7 @@ function applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch)
 		end
 
 		local targetInstance = instanceMap.fromIds[refId]
+
 		if targetInstance == nil then
 			markFailed(entry.id, entry.propertyName, entry.virtualValue)
 			continue
@@ -155,4 +147,7 @@ function applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch)
 	end
 end
 
-return reify
+return {
+	reifyInstance = reifyInstance,
+	applyDeferredRefs = applyDeferredRefs,
+}

plugin/src/Reconciler/reify.spec.lua

@@ -1,5 +1,6 @@
 return function()
 	local reify = require(script.Parent.reify)
+	local reifyInstance, applyDeferredRefs = reify.reifyInstance, reify.applyDeferredRefs
 
 	local PatchSet = require(script.Parent.Parent.PatchSet)
 	local InstanceMap = require(script.Parent.Parent.InstanceMap)
@@ -20,7 +21,11 @@ return function()
 
 	it("should throw when given a bogus ID", function()
 		expect(function()
-			reify(InstanceMap.new(), {}, "Hi, mom!", game)
+			local deferredRefs = {}
+			local instanceMap = InstanceMap.new()
+			local unappliedPatch = reifyInstance(deferredRefs, instanceMap, {}, "Hi, mom!", game)
+
+			applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch)
 		end).to.throw()
 	end)
 
@@ -34,8 +39,11 @@ return function()
 			},
 		}
 
+		local deferredRefs = {}
 		local instanceMap = InstanceMap.new()
-		local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT", nil)
+		local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT", nil)
+
+		applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch)
 
 		assert(instanceMap:size() == 0, "expected instanceMap to be empty")
 
@@ -60,8 +68,11 @@ return function()
 			},
 		}
 
+		local deferredRefs = {}
 		local instanceMap = InstanceMap.new()
-		local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT")
+		local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT")
+
+		applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch)
 
 		assert(PatchSet.isEmpty(unappliedPatch), "expected remaining patch to be empty")
 
@@ -90,8 +101,11 @@ return function()
 			},
 		}
 
+		local deferredRefs = {}
 		local instanceMap = InstanceMap.new()
-		local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT")
+		local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT")
+
+		applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch)
 
 		assert(PatchSet.isEmpty(unappliedPatch), "expected remaining patch to be empty")
 
@@ -122,8 +136,11 @@ return function()
 			},
 		}
 
+		local deferredRefs = {}
 		local instanceMap = InstanceMap.new()
-		local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT")
+		local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT")
+
+		applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch)
 
 		expect(size(unappliedPatch.added)).to.equal(1)
 		expect(unappliedPatch.added["CHILD"]).to.equal(virtualInstances["CHILD"])
@@ -153,8 +170,11 @@ return function()
 			},
 		}
 
+		local deferredRefs = {}
 		local instanceMap = InstanceMap.new()
-		local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT")
+		local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT")
+
+		applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch)
 
 		local instance = instanceMap.fromIds["ROOT"]
 		expect(instance.ClassName).to.equal("StringValue")
@@ -196,8 +216,11 @@ return function()
 			},
 		}
 
+		local deferredRefs = {}
 		local instanceMap = InstanceMap.new()
-		local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT")
+		local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT")
+
+		applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch)
 
 		assert(PatchSet.isEmpty(unappliedPatch), "expected remaining patch to be empty")
 
@@ -223,13 +246,16 @@ return function()
 			},
 		}
 
+		local deferredRefs = {}
 		local instanceMap = InstanceMap.new()
 
 		local existing = Instance.new("Folder")
 		existing.Name = "Existing"
 		instanceMap:insert("EXISTING", existing)
 
-		local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT")
+		local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT")
+
+		applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch)
 
 		assert(PatchSet.isEmpty(unappliedPatch), "expected remaining patch to be empty")
 
@@ -268,8 +294,11 @@ return function()
 			},
 		}
 
+		local deferredRefs = {}
 		local instanceMap = InstanceMap.new()
-		local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT")
+		local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT")
+
+		applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch)
 
 		assert(PatchSet.isEmpty(unappliedPatch), "expected remaining patch to be empty")
 
@@ -307,8 +336,11 @@ return function()
 			},
 		}
 
+		local deferredRefs = {}
 		local instanceMap = InstanceMap.new()
-		local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT")
+		local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT")
+
+		applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch)
 
 		assert(PatchSet.isEmpty(unappliedPatch), "expected remaining patch to be empty")
 
@@ -332,8 +364,11 @@ return function()
 			},
 		}
 
+		local deferredRefs = {}
 		local instanceMap = InstanceMap.new()
-		local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT")
+		local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT")
+
+		applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch)
 
 		assert(not PatchSet.hasRemoves(unappliedPatch), "expected no removes")
 		assert(not PatchSet.hasAdditions(unappliedPatch), "expected no additions")