From 9f382ed9bd4c21141ecd55cfb3e896d536426390 Mon Sep 17 00:00:00 2001 From: Lucien Greathouse Date: Tue, 19 Mar 2019 16:30:06 -0700 Subject: [PATCH] Iterate on plugin reconciler - Renamed setProperty to setCanonicalProperty, which is more usefully descriptive. Also added a detailed comment. - Fixed reconciler behavior with regards to removing known instances when $ignoreUnknownInstances is set --- plugin/src/Reconciler.lua | 31 ++- plugin/src/Reconciler.spec.lua | 218 ++++++++++++++++++ ...tProperty.lua => setCanonicalProperty.lua} | 15 +- 3 files changed, 248 insertions(+), 16 deletions(-) create mode 100644 plugin/src/Reconciler.spec.lua rename plugin/src/{setProperty.lua => setCanonicalProperty.lua} (69%) diff --git a/plugin/src/Reconciler.lua b/plugin/src/Reconciler.lua index e3a7bc40..d63bfddd 100644 --- a/plugin/src/Reconciler.lua +++ b/plugin/src/Reconciler.lua @@ -1,6 +1,6 @@ local InstanceMap = require(script.Parent.InstanceMap) local Logging = require(script.Parent.Logging) -local setProperty = require(script.Parent.setProperty) +local setCanonicalProperty = require(script.Parent.setCanonicalProperty) local rojoValueToRobloxValue = require(script.Parent.rojoValueToRobloxValue) local Reconciler = {} @@ -43,10 +43,10 @@ function Reconciler:reconcile(virtualInstancesById, id, instance) self.instanceMap:insert(id, instance) -- Some instances don't like being named, even if their name already matches - setProperty(instance, "Name", virtualInstance.Name) + setCanonicalProperty(instance, "Name", virtualInstance.Name) for key, value in pairs(virtualInstance.Properties) do - setProperty(instance, key, rojoValueToRobloxValue(value)) + setCanonicalProperty(instance, key, rojoValueToRobloxValue(value)) end local existingChildren = instance:GetChildren() @@ -81,10 +81,17 @@ function Reconciler:reconcile(virtualInstancesById, id, instance) end end - if self:__shouldClearUnknownInstances(virtualInstance) then - for existingChildInstance in pairs(unvisitedExistingChildren) do - self.instanceMap:removeInstance(existingChildInstance) - existingChildInstance:Destroy() + local shouldClearUnknown = self:__shouldClearUnknownChildren(virtualInstance) + + for existingChildInstance in pairs(unvisitedExistingChildren) do + local childId = self.instanceMap.fromInstances[existingChildInstance] + + if childId == nil then + if shouldClearUnknown then + existingChildInstance:Destroy() + end + else + self.instanceMap:destroyInstance(existingChildInstance) end end @@ -100,13 +107,13 @@ function Reconciler:reconcile(virtualInstancesById, id, instance) -- Some instances, like services, don't like having their Parent -- property poked, even if we're setting it to the same value. - setProperty(instance, "Parent", parent) + setCanonicalProperty(instance, "Parent", parent) end return instance end -function Reconciler:__shouldClearUnknownInstances(virtualInstance) +function Reconciler:__shouldClearUnknownChildren(virtualInstance) if virtualInstance.Metadata ~= nil then return not virtualInstance.Metadata.ignoreUnknownInstances else @@ -120,16 +127,16 @@ function Reconciler:__reify(virtualInstancesById, id, parent) local instance = Instance.new(virtualInstance.ClassName) for key, value in pairs(virtualInstance.Properties) do - setProperty(instance, key, rojoValueToRobloxValue(value)) + setCanonicalProperty(instance, key, rojoValueToRobloxValue(value)) end - instance.Name = virtualInstance.Name + setCanonicalProperty(instance, "Name", virtualInstance.Name) for _, childId in ipairs(virtualInstance.Children) do self:__reify(virtualInstancesById, childId, instance) end - setProperty(instance, "Parent", parent) + setCanonicalProperty(instance, "Parent", parent) self.instanceMap:insert(id, instance) return instance diff --git a/plugin/src/Reconciler.spec.lua b/plugin/src/Reconciler.spec.lua new file mode 100644 index 00000000..794c4d08 --- /dev/null +++ b/plugin/src/Reconciler.spec.lua @@ -0,0 +1,218 @@ +local Reconciler = require(script.Parent.Reconciler) + +return function() + it("should leave instances alone if there's nothing specified", function() + local instance = Instance.new("Folder") + instance.Name = "TestFolder" + + local instanceId = "test-id" + local virtualInstancesById = { + [instanceId] = { + Name = "TestFolder", + ClassName = "Folder", + Children = {}, + Properties = {}, + }, + } + + local reconciler = Reconciler.new() + reconciler:reconcile(virtualInstancesById, instanceId, instance) + end) + + it("should assign names from virtual instances", function() + local instance = Instance.new("Folder") + instance.Name = "InitialName" + + local instanceId = "test-id" + local virtualInstancesById = { + [instanceId] = { + Name = "NewName", + ClassName = "Folder", + Children = {}, + Properties = {}, + }, + } + + local reconciler = Reconciler.new() + reconciler:reconcile(virtualInstancesById, instanceId, instance) + + expect(instance.Name).to.equal("NewName") + end) + + it("should assign properties from virtual instances", function() + local instance = Instance.new("IntValue") + instance.Name = "TestValue" + instance.Value = 5 + + local instanceId = "test-id" + local virtualInstancesById = { + [instanceId] = { + Name = "TestValue", + ClassName = "IntValue", + Children = {}, + Properties = { + Value = { + Type = "Int32", + Value = 9 + } + }, + }, + } + + local reconciler = Reconciler.new() + reconciler:reconcile(virtualInstancesById, instanceId, instance) + + expect(instance.Value).to.equal(9) + end) + + it("should wipe unknown children by default", function() + local parent = Instance.new("Folder") + parent.Name = "Parent" + + local child = Instance.new("Folder") + child.Name = "Child" + + local parentId = "test-id" + local virtualInstancesById = { + [parentId] = { + Name = "Parent", + ClassName = "Folder", + Children = {}, + Properties = {}, + }, + } + + local reconciler = Reconciler.new() + reconciler:reconcile(virtualInstancesById, parentId, parent) + + expect(#parent:GetChildren()).to.equal(0) + end) + + it("should preserve unknown children if ignoreUnknownInstances is set", function() + local parent = Instance.new("Folder") + parent.Name = "Parent" + + local child = Instance.new("Folder") + child.Parent = parent + child.Name = "Child" + + local parentId = "test-id" + local virtualInstancesById = { + [parentId] = { + Name = "Parent", + ClassName = "Folder", + Children = {}, + Properties = {}, + Metadata = { + ignoreUnknownInstances = true, + }, + }, + } + + local reconciler = Reconciler.new() + reconciler:reconcile(virtualInstancesById, parentId, parent) + + expect(child.Parent).to.equal(parent) + expect(#parent:GetChildren()).to.equal(1) + end) + + it("should remove known removed children", function() + local parent = Instance.new("Folder") + parent.Name = "Parent" + + local child = Instance.new("Folder") + child.Parent = parent + child.Name = "Child" + + local parentId = "parent-id" + local childId = "child-id" + + local reconciler = Reconciler.new() + + local virtualInstancesById = { + [parentId] = { + Name = "Parent", + ClassName = "Folder", + Children = {childId}, + Properties = {}, + }, + [childId] = { + Name = "Child", + ClassName = "Folder", + Children = {}, + Properties = {}, + }, + } + reconciler:reconcile(virtualInstancesById, parentId, parent) + + expect(child.Parent).to.equal(parent) + expect(#parent:GetChildren()).to.equal(1) + + local newVirtualInstances = { + [parentId] = { + Name = "Parent", + ClassName = "Folder", + Children = {}, + Properties = {}, + }, + [childId] = nil, + } + reconciler:reconcile(newVirtualInstances, parentId, parent) + + expect(child.Parent).to.equal(nil) + expect(#parent:GetChildren()).to.equal(0) + end) + + it("should remove known removed children if ignoreUnknownInstances is set", function() + local parent = Instance.new("Folder") + parent.Name = "Parent" + + local child = Instance.new("Folder") + child.Parent = parent + child.Name = "Child" + + local parentId = "parent-id" + local childId = "child-id" + + local reconciler = Reconciler.new() + + local virtualInstancesById = { + [parentId] = { + Name = "Parent", + ClassName = "Folder", + Children = {childId}, + Properties = {}, + Metadata = { + ignoreUnknownInstances = true, + }, + }, + [childId] = { + Name = "Child", + ClassName = "Folder", + Children = {}, + Properties = {}, + }, + } + reconciler:reconcile(virtualInstancesById, parentId, parent) + + expect(child.Parent).to.equal(parent) + expect(#parent:GetChildren()).to.equal(1) + + local newVirtualInstances = { + [parentId] = { + Name = "Parent", + ClassName = "Folder", + Children = {}, + Properties = {}, + Metadata = { + ignoreUnknownInstances = true, + }, + }, + [childId] = nil, + } + reconciler:reconcile(newVirtualInstances, parentId, parent) + + expect(child.Parent).to.equal(nil) + expect(#parent:GetChildren()).to.equal(0) + end) +end \ No newline at end of file diff --git a/plugin/src/setProperty.lua b/plugin/src/setCanonicalProperty.lua similarity index 69% rename from plugin/src/setProperty.lua rename to plugin/src/setCanonicalProperty.lua index 76a44f82..d7e1e7d8 100644 --- a/plugin/src/setProperty.lua +++ b/plugin/src/setCanonicalProperty.lua @@ -1,10 +1,17 @@ local Logging = require(script.Parent.Logging) --[[ - Attempts to set a property on the given instance, correctly handling - 'virtual properties', which aren't reflected directly to Lua. + Attempts to set a property on the given instance. + + This method deals in terms of what Rojo calls 'canonical properties', which + don't necessarily exist either in serialization or in Lua-reflected APIs, + but may be present in the API dump. + + Ideally, canonical properties map 1:1 with properties we can assign, but in + some cases like LocalizationTable contents and CollectionService tags, we + have to read/write properties a little differently. ]] -local function setProperty(instance, key, value) +local function setCanonicalProperty(instance, key, value) -- The 'Contents' property of LocalizationTable isn't directly exposed, but -- has corresponding (deprecated) getters and setters. if instance.ClassName == "LocalizationTable" and key == "Contents" then @@ -42,4 +49,4 @@ local function setProperty(instance, key, value) return true end -return setProperty \ No newline at end of file +return setCanonicalProperty \ No newline at end of file