Break apart plugin reconciler (#332)

* Start splitting apart reconciler, with tests

* Reify children in reify

* Baseline hydrate implementation

* Remove debug print

* Scaffold out diff implementation, just supporting name changes

* invariant -> error in decodeValue

* Flesh out diff and add getProperty

* Clear out top-level reconciler interface, start updating code that touches it

* Address review feedback

* Add (experimental) Selene configuration

* Add emptiness checks to PatchSet, remove unimplement invert method

* Improve descendant destruction behavior in InstanceMap

* Track instanceId on all reify errors

* Base implementation of applyPatch, returning partial patches on failure

* Change reify to accept InstanceMap and insert instances into it

* Start testing applyPatch for removals

* Add test for applyPatch adding instances successfully and not

* Add , which is just error with formatting

* Correctly use new diff and applyPatch APIs

* Improve applyPatch logging and fix field name typo

* Better debug output when reify fails

* Print out unapplied patch in debug mode

* Don't write properties if their values are not different.

This was exposed trying to sync the Rojo plugin, which
has a gigantic ModuleScript in it with the reflection
database. This workaround was present in some form in
many versions of Rojo, and I guess we still need it.

This time, I actually documented why it's here so that
I don't forget for the umpteenth time...

* Add placeholder test that needs to happen still

* Introduce easier plugin testing, write applyPatch properties test

* Delete legacy get/setCanonicalProperty files

* Fix trying to remove numbers instead of instances

* Change applyPatch to return partial patches instead of binary success

* Work towards being able to decode and apply refs

* Add helpers for PatchSet assertions

* Apply refs in reify, test all cases

* Improve diagnostics when patches fail to apply

* Stop logging when destroying untracked instances, it's ok

* Remove read before setting property in applyPatch

* Fix diff thinking all properties are changed

plugin/src/Reconciler/reify.lua

@@ -0,0 +1,152 @@
+--[[
+	"Reifies" a virtual DOM, constructing a real DOM with the same shape.
+]]
+
+local invariant = require(script.Parent.Parent.invariant)
+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.
+]]
+local function addAllToPatch(patchSet, virtualInstances, id)
+	local virtualInstance = virtualInstances[id]
+	patchSet.added[id] = virtualInstance
+
+	for _, childId in ipairs(virtualInstance.Children) do
+		addAllToPatch(patchSet, virtualInstances, childId)
+	end
+end
+
+--[[
+	Inner function that defines the core routine.
+]]
+function reifyInner(instanceMap, virtualInstances, id, parentInstance, unappliedPatch, deferredRefs)
+	local virtualInstance = virtualInstances[id]
+
+	if virtualInstance == nil then
+		invariant("Cannot reify an instance not present in virtualInstances\nID: {}", id)
+	end
+
+	-- Instance.new can fail if we're passing in something that can't be
+	-- created, like a service, something enabled with a feature flag, or
+	-- something that requires higher security than we have.
+	local ok, instance = pcall(Instance.new, virtualInstance.ClassName)
+
+	if not ok then
+		addAllToPatch(unappliedPatch, virtualInstances, id)
+		return
+	end
+
+	-- TODO: Can this fail? Previous versions of Rojo guarded against this, but
+	-- the reason why was uncertain.
+	instance.Name = virtualInstance.Name
+
+	-- Track all of the properties that we've failed to assign to this instance.
+	local unappliedProperties = {}
+
+	for propertyName, virtualValue in pairs(virtualInstance.Properties) 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 virtualValue.Type == "Ref" then
+			table.insert(deferredRefs, {
+				id = id,
+				instance = instance,
+				propertyName = propertyName,
+				virtualValue = virtualValue,
+			})
+			continue
+		end
+
+		local ok, value = decodeValue(virtualValue, instanceMap)
+		if not ok then
+			unappliedProperties[propertyName] = virtualValue
+			continue
+		end
+
+		local ok = setProperty(instance, propertyName, value)
+		if not ok then
+			unappliedProperties[propertyName] = virtualValue
+		end
+	end
+
+	-- If there were any properties that we failed to assign, push this into our
+	-- unapplied patch as an update that would need to be applied.
+	if next(unappliedProperties) ~= nil then
+		table.insert(unappliedPatch.updated, {
+			id = id,
+			changedProperties = unappliedProperties,
+		})
+	end
+
+	for _, childId in ipairs(virtualInstance.Children) do
+		reifyInner(instanceMap, virtualInstances, childId, instance, unappliedPatch, deferredRefs)
+	end
+
+	instance.Parent = parentInstance
+	instanceMap:insert(id, instance)
+end
+
+function applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch)
+	local function markFailed(id, propertyName, virtualValue)
+		-- If there is already an updated entry in the unapplied patch for this
+		-- ref, use the existing one. This could match other parts of the
+		-- instance that failed to be created, or even just other refs that
+		-- failed to apply.
+		--
+		-- This is important for instances like selectable GUI objects, which
+		-- have many similar referent properties.
+		for _, existingUpdate in ipairs(unappliedPatch.updated) do
+			if existingUpdate.id == id then
+				existingUpdate.changedProperties[propertyName] = virtualValue
+				return
+			end
+		end
+
+		-- We didn't find an existing entry that matched, so push a new entry
+		-- into our unapplied patch.
+		table.insert(unappliedPatch.updated, {
+			id = id,
+			changedProperties = {
+				[propertyName] = virtualValue,
+			},
+		})
+	end
+
+	for _, entry in ipairs(deferredRefs) do
+		local targetInstance = instanceMap.fromIds[entry.virtualValue.Value]
+		if targetInstance == nil then
+			markFailed(entry.id, entry.propertyName, entry.virtualValue)
+			continue
+		end
+
+		local ok = setProperty(entry.instance, entry.propertyName, targetInstance)
+		if not ok then
+			markFailed(entry.id, entry.propertyName, entry.virtualValue)
+		end
+	end
+end
+
+return reify