From f66860bdfe1b6a5f0174465c0a6c964b56e304cb Mon Sep 17 00:00:00 2001 From: Lucien Greathouse Date: Wed, 11 Nov 2020 16:30:23 -0800 Subject: [PATCH] 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 --- .gitignore | 8 +- .luacheckrc | 2 +- foreman.toml | 1 + plugin/log/init.lua | 4 + ...tstrap.server.lua => run-tests.server.lua} | 0 plugin/src/InstanceMap.lua | 38 +- plugin/src/PatchSet.lua | 165 +++++++- plugin/src/Reconciler.lua | 389 ------------------ plugin/src/Reconciler/Error.lua | 37 ++ plugin/src/Reconciler/applyPatch.lua | 131 ++++++ plugin/src/Reconciler/applyPatch.spec.lua | 159 +++++++ plugin/src/Reconciler/decodeValue.lua | 35 ++ plugin/src/Reconciler/diff.lua | 148 +++++++ plugin/src/Reconciler/diff.spec.lua | 292 +++++++++++++ .../getProperty.lua} | 31 +- plugin/src/Reconciler/hydrate.lua | 50 +++ plugin/src/Reconciler/hydrate.spec.lua | 129 ++++++ plugin/src/Reconciler/init.lua | 34 ++ plugin/src/Reconciler/reify.lua | 152 +++++++ plugin/src/Reconciler/reify.spec.lua | 352 ++++++++++++++++ plugin/src/Reconciler/setProperty.lua | 48 +++ plugin/src/ServeSession.lua | 32 +- plugin/src/setCanonicalProperty.lua | 37 -- plugin/test | 4 + plugin/test-place.project.json | 2 +- selene.toml | 4 + 26 files changed, 1818 insertions(+), 466 deletions(-) rename plugin/{testBootstrap.server.lua => run-tests.server.lua} (100%) delete mode 100644 plugin/src/Reconciler.lua create mode 100644 plugin/src/Reconciler/Error.lua create mode 100644 plugin/src/Reconciler/applyPatch.lua create mode 100644 plugin/src/Reconciler/applyPatch.spec.lua create mode 100644 plugin/src/Reconciler/decodeValue.lua create mode 100644 plugin/src/Reconciler/diff.lua create mode 100644 plugin/src/Reconciler/diff.spec.lua rename plugin/src/{getCanonicalProperty.lua => Reconciler/getProperty.lua} (55%) create mode 100644 plugin/src/Reconciler/hydrate.lua create mode 100644 plugin/src/Reconciler/hydrate.spec.lua create mode 100644 plugin/src/Reconciler/init.lua create mode 100644 plugin/src/Reconciler/reify.lua create mode 100644 plugin/src/Reconciler/reify.spec.lua create mode 100644 plugin/src/Reconciler/setProperty.lua delete mode 100644 plugin/src/setCanonicalProperty.lua create mode 100644 plugin/test create mode 100644 selene.toml diff --git a/.gitignore b/.gitignore index 3b6e942e..b12513e1 100644 --- a/.gitignore +++ b/.gitignore @@ -10,9 +10,15 @@ /*.rbxl /*.rbxlx +# Test places for the Roblox Studio Plugin +/plugin/*.rbxlx + # Roblox Studio holds 'lock' files on places *.rbxl.lock *.rbxlx.lock # Snapshot files from the 'insta' Rust crate -**/*.snap.new \ No newline at end of file +**/*.snap.new + +# Selene generates a roblox.toml file that should not be checked in. +/roblox.toml \ No newline at end of file diff --git a/.luacheckrc b/.luacheckrc index b523c1b5..bb55667f 100644 --- a/.luacheckrc +++ b/.luacheckrc @@ -33,7 +33,7 @@ stds.plugin = { stds.testez = { read_globals = { "describe", - "it", "itFOCUS", "itSKIP", + "it", "itFOCUS", "itSKIP", "itFIXME", "FOCUS", "SKIP", "HACK_NO_XPCALL", "expect", } diff --git a/foreman.toml b/foreman.toml index fe2956e1..53464e41 100644 --- a/foreman.toml +++ b/foreman.toml @@ -1,2 +1,3 @@ [tools] rojo = { source = "rojo-rbx/rojo", version = "6.0.0-rc.1" } +run-in-roblox = { source = "rojo-rbx/run-in-roblox", version = "0.3.0" } diff --git a/plugin/log/init.lua b/plugin/log/init.lua index dae88594..97fa7e46 100644 --- a/plugin/log/init.lua +++ b/plugin/log/init.lua @@ -53,4 +53,8 @@ function Log.warn(template, ...) end end +function Log.error(template, ...) + error(Fmt.fmt(template, ...)) +end + return Log \ No newline at end of file diff --git a/plugin/testBootstrap.server.lua b/plugin/run-tests.server.lua similarity index 100% rename from plugin/testBootstrap.server.lua rename to plugin/run-tests.server.lua diff --git a/plugin/src/InstanceMap.lua b/plugin/src/InstanceMap.lua index 68f8c219..fe2b3eac 100644 --- a/plugin/src/InstanceMap.lua +++ b/plugin/src/InstanceMap.lua @@ -33,6 +33,16 @@ function InstanceMap.new(onInstanceChanged) return setmetatable(self, InstanceMap) end +function InstanceMap:size() + local size = 0 + + for _ in pairs(self.fromIds) do + size = size + 1 + end + + return size +end + --[[ Disconnect all connections and release all instance references. ]] @@ -81,8 +91,6 @@ function InstanceMap:removeId(id) self:__disconnectSignals(instance) self.fromIds[id] = nil self.fromInstances[instance] = nil - else - Log.warn("Attempted to remove nonexistant ID {}", id) end end @@ -93,8 +101,6 @@ function InstanceMap:removeInstance(instance) if id ~= nil then self.fromInstances[instance] = nil self.fromIds[id] = nil - else - Log.warn("Attempted to remove nonexistant instance {}", instance) end end @@ -102,10 +108,14 @@ function InstanceMap:destroyInstance(instance) local id = self.fromInstances[instance] if id ~= nil then - self:destroyId(id) - else - Log.warn("Attempted to destroy untracked instance {}", instance) + self:removeId(id) end + + for _, descendantInstance in ipairs(instance:GetDescendants()) do + self:removeInstance(descendantInstance) + end + + instance:Destroy() end function InstanceMap:destroyId(id) @@ -113,21 +123,11 @@ function InstanceMap:destroyId(id) self:removeId(id) if instance ~= nil then - local descendantsToDestroy = {} - - for otherInstance in pairs(self.fromInstances) do - if otherInstance:IsDescendantOf(instance) then - table.insert(descendantsToDestroy, otherInstance) - end - end - - for _, otherInstance in ipairs(descendantsToDestroy) do - self:removeInstance(otherInstance) + for _, descendantInstance in ipairs(instance:GetDescendants()) do + self:removeInstance(descendantInstance) end instance:Destroy() - else - Log.warn("Attempted to destroy nonexistant ID {}", id) end end diff --git a/plugin/src/PatchSet.lua b/plugin/src/PatchSet.lua index 6d18b46d..d8fde747 100644 --- a/plugin/src/PatchSet.lua +++ b/plugin/src/PatchSet.lua @@ -16,10 +16,169 @@ PatchSet.validate = t.interface({ }) --[[ - Invert the given PatchSet using the given instance map. + Create a new, empty PatchSet. ]] -function PatchSet.invert(patchSet, instanceMap) - error("not yet implemented", 2) +function PatchSet.newEmpty() + return { + removed = {}, + added = {}, + updated = {}, + } +end + +--[[ + Tells whether the given PatchSet is empty. +]] +function PatchSet.isEmpty(patchSet) + return next(patchSet.removed) == nil and + next(patchSet.added) == nil and + next(patchSet.updated) == nil +end + +--[[ + Tells whether the given PatchSet has any remove operations. +]] +function PatchSet.hasRemoves(patchSet) + return next(patchSet.removed) ~= nil +end + +--[[ + Tells whether the given PatchSet has any add operations. +]] +function PatchSet.hasAdditions(patchSet) + return next(patchSet.added) ~= nil +end + +--[[ + Tells whether the given PatchSet has any update operations. +]] +function PatchSet.hasUpdates(patchSet) + return next(patchSet.updated) ~= nil +end + +--[[ + Merge multiple PatchSet objects into the given PatchSet. +]] +function PatchSet.assign(target, ...) + for i = 1, select("#", ...) do + local sourcePatch = select(i, ...) + + for _, removed in ipairs(sourcePatch.removed) do + table.insert(target.removed, removed) + end + + for id, added in pairs(sourcePatch.added) do + target.added[id] = added + end + + for _, update in ipairs(sourcePatch.updated) do + table.insert(target.updated, update) + end + end + + return target +end + +--[[ + Create a list of human-readable statements summarizing the contents of this + patch, intended to be displayed to users. +]] +function PatchSet.humanSummary(instanceMap, patchSet) + local statements = {} + + for _, idOrInstance in ipairs(patchSet.removed) do + local instance, id + + if type(idOrInstance) == "string" then + id = idOrInstance + instance = instanceMap.fromIds[id] + else + instance = idOrInstance + id = instanceMap.fromInstances[instance] + end + + if instance ~= nil then + table.insert(statements, string.format("- Delete instance %s", instance:GetFullName())) + else + table.insert(statements, string.format("- Delete instance with ID %s", id)) + end + end + + local additionsMentioned = {} + + local function addAllDescendents(virtualInstance) + additionsMentioned[virtualInstance.Id] = true + + for _, childId in ipairs(virtualInstance.Children) do + addAllDescendents(patchSet.added[childId]) + end + end + + for id, addition in pairs(patchSet.added) do + if additionsMentioned[id] then + continue + end + + local virtualInstance = addition + while true do + if virtualInstance.Parent == nil then + break + end + + local virtualParent = patchSet.added[virtualInstance.Parent] + if virtualParent == nil then + break + end + + virtualInstance = virtualParent + end + + local parentDisplayName = "nil (how strange!)" + if virtualInstance.Parent ~= nil then + local parent = instanceMap.fromIds[virtualInstance.Parent] + if parent ~= nil then + parentDisplayName = parent:GetFullName() + end + end + + table.insert(statements, string.format( + "- Add instance %q (ClassName %q) to %s", + virtualInstance.Name, virtualInstance.ClassName, parentDisplayName)) + end + + for _, update in ipairs(patchSet.updated) do + local updatedProperties = {} + + if update.changedMetadata ~= nil then + table.insert(updatedProperties, "Rojo's Metadata") + end + + if update.changedName ~= nil then + table.insert(updatedProperties, "Name") + end + + if update.changedClassName ~= nil then + table.insert(updatedProperties, "ClassName") + end + + for name in pairs(update.changedProperties) do + table.insert(updatedProperties, name) + end + + local instance = instanceMap.fromIds[update.id] + local displayName + if instance ~= nil then + displayName = instance:GetFullName() + else + displayName = "[unknown instance]" + end + + table.insert(statements, string.format( + "- Update properties on %s: %s", + displayName, table.concat(updatedProperties, ","))) + end + + return table.concat(statements, "\n") end return PatchSet \ No newline at end of file diff --git a/plugin/src/Reconciler.lua b/plugin/src/Reconciler.lua deleted file mode 100644 index ad395abf..00000000 --- a/plugin/src/Reconciler.lua +++ /dev/null @@ -1,389 +0,0 @@ ---[[ - This module defines the meat of the Rojo plugin and how it manages tracking - and mutating the Roblox DOM. -]] - -local RbxDom = require(script.Parent.Parent.RbxDom) -local t = require(script.Parent.Parent.t) - -local Types = require(script.Parent.Types) -local invariant = require(script.Parent.invariant) -local getCanonicalProperty = require(script.Parent.getCanonicalProperty) -local setCanonicalProperty = require(script.Parent.setCanonicalProperty) -local PatchSet = require(script.Parent.PatchSet) - ---[[ - Attempt to safely set the parent of an instance. - - This function will always succeed, even if the actual set failed. This is - important for some types like services that will throw even if their current - parent is already set to the requested parent. - - TODO: See if we can eliminate this by being more nuanced with property - assignment? -]] -local function safeSetParent(instance, newParent) - pcall(function() - instance.Parent = newParent - end) -end - ---[[ - Similar to setting Parent, some instances really don't like being renamed. - - TODO: Should we be throwing away these results or can we be more careful? -]] -local function safeSetName(instance, name) - pcall(function() - instance.Name = name - end) -end - -local Reconciler = {} -Reconciler.__index = Reconciler - -function Reconciler.new(instanceMap) - local self = { - -- Tracks all of the instances known by the reconciler by ID. - __instanceMap = instanceMap, - } - - return setmetatable(self, Reconciler) -end - ---[[ - See Reconciler:__hydrateInternal(). -]] -function Reconciler:hydrate(apiInstances, id, instance) - local hydratePatch = { - removed = {}, - added = {}, - updated = {}, - } - - self:__hydrateInternal(apiInstances, id, instance, hydratePatch) - - return hydratePatch -end - ---[[ - Applies a patch to the Roblox DOM using the reconciler's internal state. - - TODO: This function might only apply some of the patch in the future and - require content negotiation with the Rojo server to handle types that aren't - editable by scripts. -]] -local applyPatchSchema = Types.ifEnabled(t.tuple( - PatchSet.validate -)) -function Reconciler:applyPatch(patch) - assert(applyPatchSchema(patch)) - - for _, removedIdOrInstance in ipairs(patch.removed) do - local removedInstance - - if Types.RbxId(removedIdOrInstance) then - -- If this value is an ID, it's assumed to be an instance that the - -- Rojo server knows about. - removedInstance = self.__instanceMap.fromIds[removedIdOrInstance] - self.__instanceMap:removeId(removedIdOrInstance) - end - - -- If this entry was an ID that we didn't know about, removedInstance - -- will be nil, which we guard against in case of minor tree desync. - if removedInstance ~= nil then - -- Ensure that if any descendants are tracked by Rojo, that we - -- properly un-track them. - for _, descendantInstance in ipairs(removedInstance:GetDescendants()) do - self.__instanceMap:removeInstance(descendantInstance) - end - - removedInstance:Destroy() - end - end - - -- TODO: This loop assumes that apiInstance.ParentId is never nil. The Rojo - -- plugin can't create a new top-level DataModel anyways, so this should - -- only be violated in cases that are already erroneous. - for id, apiInstance in pairs(patch.added) do - if self.__instanceMap.fromIds[id] == nil then - -- Find the first ancestor of this instance that is marked for an - -- addition. - -- - -- This helps us make sure we only reify each instance once, and we - -- start from the top. - while patch.added[apiInstance.Parent] ~= nil do - id = apiInstance.Parent - apiInstance = patch.added[id] - end - - local parentInstance = self.__instanceMap.fromIds[apiInstance.Parent] - - if parentInstance == nil then - invariant( - "Cannot add an instance from a patch that has no parent.\nInstance {} with parent {}.\nState: {:#?}", - id, - apiInstance.Parent, - self.__instanceMap - ) - end - - self:__reifyInstance(patch.added, id, parentInstance) - end - end - - for _, update in ipairs(patch.updated) do - local instance = self.__instanceMap.fromIds[update.id] - - if instance == nil then - invariant( - "Cannot update an instance that does not exist in the reconciler's state.\nInstance {}\nState: {:#?}", - update.id, - self.__instanceMap - ) - end - - if update.changedClassName ~= nil then - error("TODO: Support changing class name by destroying + recreating instance.") - end - - if update.changedName ~= nil then - instance.Name = update.changedName - end - - if update.changedMetadata ~= nil then - print("TODO: Support changing metadata, if necessary.") - end - - if update.changedProperties ~= nil then - for propertyName, propertyValue in pairs(update.changedProperties) do - -- TODO: Gracefully handle this error instead? - assert(setCanonicalProperty(instance, propertyName, self:__decodeApiValue(propertyValue))) - end - end - end -end - ---[[ - Transforms a value into one that can be sent over the network back to the - Rojo server. - - This operation can fail, and so it returns bool, value. -]] -function Reconciler:encodeApiValue(value) - if typeof(value) == "string" then - return true, { - Type = "String", - Value = value, - } - end - - return false -end - ---[[ - Transforms a value encoded by rbx_dom_weak on the server side into a value - usable by Rojo's reconciler, potentially using RbxDom. -]] -function Reconciler:__decodeApiValue(apiValue) - assert(Types.ApiValue(apiValue)) - - -- Refs are represented as IDs in the same space that Rojo's protocol uses. - if apiValue.Type == "Ref" then - -- TODO: This ref could be pointing at an instance we haven't created - -- yet! - - return self.__instanceMap.fromIds[apiValue.Value] - end - - local success, decodedValue = RbxDom.EncodedValue.decode(apiValue) - - if not success then - error(decodedValue, 2) - end - - return decodedValue -end - ---[[ - Constructs an instance from an ApiInstance without any of its children. -]] -local reifySingleInstanceSchema = Types.ifEnabled(t.tuple( - Types.ApiInstance -)) -function Reconciler:__reifySingleInstance(apiInstance) - assert(reifySingleInstanceSchema(apiInstance)) - - -- 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, apiInstance.ClassName) - if not ok then - return false, instance - end - - -- TODO: When can setting Name fail here? - safeSetName(instance, apiInstance.Name) - - for key, value in pairs(apiInstance.Properties) do - setCanonicalProperty(instance, key, self:__decodeApiValue(value)) - end - - return true, instance -end - ---[[ - Construct an instance and all of its descendants, parent it to the given - instance, and insert it into the reconciler's internal state. -]] -local reifyInstanceSchema = Types.ifEnabled(t.tuple( - t.map(Types.RbxId, Types.VirtualInstance), - Types.RbxId, - t.Instance -)) -function Reconciler:__reifyInstance(apiInstances, id, parentInstance) - assert(reifyInstanceSchema(apiInstances, id, parentInstance)) - - local apiInstance = apiInstances[id] - local ok, instance = self:__reifySingleInstance(apiInstance) - - -- TODO: Propagate this error upward to handle it elsewhere? - if not ok then - error(("Couldn't create an instance of type %q, a child of %s"):format( - apiInstance.ClassName, - parentInstance:GetFullName() - )) - end - - self.__instanceMap:insert(id, instance) - - for _, childId in ipairs(apiInstance.Children) do - self:__reifyInstance(apiInstances, childId, instance) - end - - safeSetParent(instance, parentInstance) - - return instance -end - ---[[ - Populates the reconciler's internal state, maps IDs to instances that the - Rojo plugin knows about, and generates a patch that would update the Roblox - tree to match Rojo's view of the tree. -]] -local hydrateSchema = Types.ifEnabled(t.tuple( - t.map(Types.RbxId, Types.VirtualInstance), - Types.RbxId, - t.Instance, - PatchSet.validate -)) -function Reconciler:__hydrateInternal(apiInstances, id, instance, hydratePatch) - assert(hydrateSchema(apiInstances, id, instance, hydratePatch)) - - self.__instanceMap:insert(id, instance) - - local apiInstance = apiInstances[id] - - local function markIdAdded(id) - local apiInstance = apiInstances[id] - hydratePatch.added[id] = apiInstance - - for _, childId in ipairs(apiInstance.Children) do - markIdAdded(childId) - end - end - - local changedName = nil - local changedProperties = {} - - if apiInstance.Name ~= instance.Name then - changedName = apiInstance.Name - end - - for propertyName, virtualValue in pairs(apiInstance.Properties) do - local success, existingValue = getCanonicalProperty(instance, propertyName) - - if success then - local decodedValue = self:__decodeApiValue(virtualValue) - - if existingValue ~= decodedValue then - changedProperties[propertyName] = virtualValue - end - end - end - - -- If any properties differed from the virtual instance we read, add it to - -- the hydrate patch so that we can catch up. - if changedName ~= nil or next(changedProperties) ~= nil then - table.insert(hydratePatch.updated, { - id = id, - changedName = changedName, - changedClassName = nil, - changedProperties = changedProperties, - changedMetadata = nil, - }) - end - - local existingChildren = instance:GetChildren() - - -- For each existing child, we'll track whether it's been paired with an - -- instance that the Rojo server knows about. - local isExistingChildVisited = {} - for i = 1, #existingChildren do - isExistingChildVisited[i] = false - end - - for _, childId in ipairs(apiInstance.Children) do - local apiChild = apiInstances[childId] - - local childInstance - - for childIndex, instance in ipairs(existingChildren) do - if not isExistingChildVisited[childIndex] then - -- We guard accessing Name and ClassName in order to avoid - -- tripping over children of DataModel that Rojo won't have - -- permissions to access at all. - local ok, name, className = pcall(function() - return instance.Name, instance.ClassName - end) - - -- This rule is very conservative and could be loosened in the - -- future, or more heuristics could be introduced. - if ok and name == apiChild.Name and className == apiChild.ClassName then - childInstance = instance - isExistingChildVisited[childIndex] = true - break - end - end - end - - if childInstance ~= nil then - -- We found an instance that matches the instance from the API, yay! - self:__hydrateInternal(apiInstances, childId, childInstance, hydratePatch) - else - markIdAdded(childId) - end - end - - -- Any unvisited children at this point aren't known by Rojo and we can - -- destroy them unless the user has explicitly asked us to preserve children - -- of this instance. - local shouldClearUnknown = self:__shouldClearUnknownChildren(apiInstance) - if shouldClearUnknown then - for childIndex, visited in ipairs(isExistingChildVisited) do - if not visited then - table.insert(hydratePatch.removed, existingChildren[childIndex]) - end - end - end -end - -function Reconciler:__shouldClearUnknownChildren(apiInstance) - if apiInstance.Metadata ~= nil then - return not apiInstance.Metadata.ignoreUnknownInstances - else - return true - end -end - -return Reconciler \ No newline at end of file diff --git a/plugin/src/Reconciler/Error.lua b/plugin/src/Reconciler/Error.lua new file mode 100644 index 00000000..5833420a --- /dev/null +++ b/plugin/src/Reconciler/Error.lua @@ -0,0 +1,37 @@ +--[[ + Defines the errors that can be returned by the reconciler. +]] + +local Fmt = require(script.Parent.Parent.Parent.Fmt) + +local Error = {} + +local function makeVariant(name) + Error[name] = setmetatable({}, { + __tostring = function() + return "Error." .. name + end, + }) +end + +makeVariant("CannotCreateInstance") +makeVariant("CannotDecodeValue") +makeVariant("LackingPropertyPermissions") +makeVariant("OtherPropertyError") +makeVariant("RefDidNotExist") +makeVariant("UnknownProperty") +makeVariant("UnreadableProperty") +makeVariant("UnwritableProperty") + +function Error.new(kind, details) + return setmetatable({ + kind = kind, + details = details, + }, Error) +end + +function Error:__tostring() + return Fmt.fmt("Error({}): {:#?}", self.kind, self.details) +end + +return Error \ No newline at end of file diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua new file mode 100644 index 00000000..dafaa352 --- /dev/null +++ b/plugin/src/Reconciler/applyPatch.lua @@ -0,0 +1,131 @@ +--[[ + Apply a patch to the DOM. Returns any portions of the patch that weren't + possible to apply. + + Patches can come from the server or be generated by the client. +]] + +local Log = require(script.Parent.Parent.Parent.Log) + +local PatchSet = require(script.Parent.Parent.PatchSet) +local Types = require(script.Parent.Parent.Types) +local invariant = require(script.Parent.Parent.invariant) + +local decodeValue = require(script.Parent.decodeValue) +local getProperty = require(script.Parent.getProperty) +local reify = require(script.Parent.reify) +local setProperty = require(script.Parent.setProperty) + +local function applyPatch(instanceMap, patch) + -- Tracks any portions of the patch that could not be applied to the DOM. + local unappliedPatch = PatchSet.newEmpty() + + for _, removedIdOrInstance in ipairs(patch.removed) do + if Types.RbxId(removedIdOrInstance) then + instanceMap:destroyId(removedIdOrInstance) + else + instanceMap:destroyInstance(removedIdOrInstance) + end + end + + for id, virtualInstance in pairs(patch.added) do + if instanceMap.fromIds[id] ~= nil then + -- This instance already exists. We might've already added it in a + -- previous iteration of this loop, or maybe this patch was not + -- supposed to list this instance. + -- + -- It's probably fine, right? + continue + end + + -- Find the first ancestor of this instance that is marked for an + -- addition. + -- + -- This helps us make sure we only reify each instance once, and we + -- start from the top. + while patch.added[virtualInstance.Parent] ~= nil do + id = virtualInstance.Parent + virtualInstance = patch.added[id] + end + + local parentInstance = instanceMap.fromIds[virtualInstance.Parent] + + if parentInstance == nil then + -- This would be peculiar. If you create an instance with no + -- parent, were you supposed to create it at all? + invariant( + "Cannot add an instance from a patch that has no parent.\nInstance {} with parent {}.\nState: {:#?}", + id, + virtualInstance.Parent, + instanceMap + ) + end + + local failedToReify = reify(instanceMap, patch.added, id, parentInstance) + + if not PatchSet.isEmpty(failedToReify) then + Log.debug("Failed to reify as part of applying a patch: {}", failedToReify) + PatchSet.assign(unappliedPatch, failedToReify) + end + end + + for _, update in ipairs(patch.updated) do + local instance = instanceMap.fromIds[update.id] + + if instance == nil then + -- We can't update an instance that doesn't exist. + -- TODO: Should this be an invariant? + continue + end + + -- Track any part of this update that could not be applied. + local unappliedUpdate = { + id = update.id, + changedProperties = {}, + } + local partiallyApplied = false + + if update.changedClassName ~= nil then + -- TODO: Support changing class name by destroying + recreating. + unappliedUpdate.changedClassName = update.changedClassName + partiallyApplied = true + end + + if update.changedName ~= nil then + instance.Name = update.changedName + end + + if update.changedMetadata ~= nil then + -- TODO: Support changing metadata. This will become necessary when + -- Rojo persistently tracks metadata for each instance in order to + -- remove extra instances. + unappliedUpdate.changedMetadata = update.changedMetadata + partiallyApplied = true + end + + if update.changedProperties ~= nil then + for propertyName, propertyValue in pairs(update.changedProperties) do + local ok, decodedValue = decodeValue(propertyValue, instanceMap) + if not ok then + unappliedUpdate.changedProperties[propertyName] = propertyValue + partiallyApplied = true + continue + end + + local ok = setProperty(instance, propertyName, decodedValue) + if not ok then + unappliedUpdate.changedProperties[propertyName] = propertyValue + partiallyApplied = true + end + end + end + + if partiallyApplied then + table.insert(unappliedPatch.updated, unappliedUpdate) + end + end + + return unappliedPatch +end + +return applyPatch \ No newline at end of file diff --git a/plugin/src/Reconciler/applyPatch.spec.lua b/plugin/src/Reconciler/applyPatch.spec.lua new file mode 100644 index 00000000..5308079d --- /dev/null +++ b/plugin/src/Reconciler/applyPatch.spec.lua @@ -0,0 +1,159 @@ +return function() + local applyPatch = require(script.Parent.applyPatch) + + local InstanceMap = require(script.Parent.Parent.InstanceMap) + local PatchSet = require(script.Parent.Parent.PatchSet) + + local dummy = Instance.new("Folder") + local function wasDestroyed(instance) + -- If an instance was destroyed, its parent property is locked. + local ok = pcall(function() + local oldParent = instance.Parent + instance.Parent = dummy + instance.Parent = oldParent + end) + + return not ok + end + + it("should return an empty patch if given an empty patch", function() + local patch = applyPatch(InstanceMap.new(), PatchSet.newEmpty()) + assert(PatchSet.isEmpty(patch), "expected remaining patch to be empty") + end) + + it("should destroy instances listed for remove", function() + local root = Instance.new("Folder") + + local child = Instance.new("Folder") + child.Name = "Child" + child.Parent = root + + local instanceMap = InstanceMap.new() + instanceMap:insert("ROOT", root) + instanceMap:insert("CHILD", child) + + local patch = PatchSet.newEmpty() + table.insert(patch.removed, child) + + local unapplied = applyPatch(instanceMap, patch) + assert(PatchSet.isEmpty(unapplied), "expected remaining patch to be empty") + + assert(not wasDestroyed(root), "expected root to be left alone") + assert(wasDestroyed(child), "expected child to be destroyed") + + instanceMap:stop() + end) + + it("should destroy IDs listed for remove", function() + local root = Instance.new("Folder") + + local child = Instance.new("Folder") + child.Name = "Child" + child.Parent = root + + local instanceMap = InstanceMap.new() + instanceMap:insert("ROOT", root) + instanceMap:insert("CHILD", child) + + local patch = PatchSet.newEmpty() + table.insert(patch.removed, "CHILD") + + local unapplied = applyPatch(instanceMap, patch) + assert(PatchSet.isEmpty(unapplied), "expected remaining patch to be empty") + expect(instanceMap:size()).to.equal(1) + + assert(not wasDestroyed(root), "expected root to be left alone") + assert(wasDestroyed(child), "expected child to be destroyed") + + instanceMap:stop() + end) + + it("should add instances to the DOM", function() + -- Many of the details of this functionality are instead covered by + -- tests on reify, not here. + + local root = Instance.new("Folder") + + local instanceMap = InstanceMap.new() + instanceMap:insert("ROOT", root) + + local patch = PatchSet.newEmpty() + patch.added["CHILD"] = { + Id = "CHILD", + ClassName = "Model", + Name = "Child", + Parent = "ROOT", + Children = {"GRANDCHILD"}, + Properties = {}, + } + + patch.added["GRANDCHILD"] = { + Id = "GRANDCHILD", + ClassName = "Part", + Name = "Grandchild", + Parent = "CHILD", + Children = {}, + Properties = {}, + } + + local unapplied = applyPatch(instanceMap, patch) + assert(PatchSet.isEmpty(unapplied), "expected remaining patch to be empty") + expect(instanceMap:size()).to.equal(3) + + local child = root:FindFirstChild("Child") + expect(child).to.be.ok() + expect(child.ClassName).to.equal("Model") + expect(child).to.equal(instanceMap.fromIds["CHILD"]) + + local grandchild = child:FindFirstChild("Grandchild") + expect(grandchild).to.be.ok() + expect(grandchild.ClassName).to.equal("Part") + expect(grandchild).to.equal(instanceMap.fromIds["GRANDCHILD"]) + end) + + it("should return unapplied additions when instances cannot be created", function() + local root = Instance.new("Folder") + + local instanceMap = InstanceMap.new() + instanceMap:insert("ROOT", root) + + local patch = PatchSet.newEmpty() + patch.added["OOPSIE"] = { + Id = "OOPSIE", + -- Hopefully Roblox never makes an instance with this ClassName. + ClassName = "UH OH", + Name = "FUBAR", + Parent = "ROOT", + Children = {}, + Properties = {}, + } + + local unapplied = applyPatch(instanceMap, patch) + expect(unapplied.added["OOPSIE"]).to.equal(patch.added["OOPSIE"]) + expect(instanceMap:size()).to.equal(1) + expect(#root:GetChildren()).to.equal(0) + end) + + it("should apply property changes to instances", function() + local value = Instance.new("StringValue") + value.Value = "HELLO" + + local instanceMap = InstanceMap.new() + instanceMap:insert("VALUE", value) + + local patch = PatchSet.newEmpty() + table.insert(patch.updated, { + id = "VALUE", + changedProperties = { + Value = { + Type = "String", + Value = "WORLD", + }, + }, + }) + + local unapplied = applyPatch(instanceMap, patch) + assert(PatchSet.isEmpty(unapplied), "expected remaining patch to be empty") + expect(value.Value).to.equal("WORLD") + end) +end \ No newline at end of file diff --git a/plugin/src/Reconciler/decodeValue.lua b/plugin/src/Reconciler/decodeValue.lua new file mode 100644 index 00000000..3dfd321c --- /dev/null +++ b/plugin/src/Reconciler/decodeValue.lua @@ -0,0 +1,35 @@ +--[[ + Transforms a value encoded by rbx_dom_weak on the server side into a value + usable by Rojo's reconciler, potentially using RbxDom. +]] + +local RbxDom = require(script.Parent.Parent.Parent.RbxDom) +local Error = require(script.Parent.Error) + +local function decodeValue(virtualValue, instanceMap) + -- Refs are represented as IDs in the same space that Rojo's protocol uses. + if virtualValue.Type == "Ref" then + local instance = instanceMap.fromIds[virtualValue.Value] + + if instance ~= nil then + return true, instance + else + return false, Error.new(Error.RefDidNotExist, { + virtualValue = virtualValue, + }) + end + end + + local ok, decodedValue = RbxDom.EncodedValue.decode(virtualValue) + + if not ok then + return false, Error.new(Error.CannotDecodeValue, { + virtualValue = virtualValue, + innerError = decodedValue, + }) + end + + return true, decodedValue +end + +return decodeValue \ No newline at end of file diff --git a/plugin/src/Reconciler/diff.lua b/plugin/src/Reconciler/diff.lua new file mode 100644 index 00000000..67c8c161 --- /dev/null +++ b/plugin/src/Reconciler/diff.lua @@ -0,0 +1,148 @@ +--[[ + Defines the process for diffing a virtual DOM and the real DOM to compute a + patch that can be later applied. +]] + +local Log = require(script.Parent.Parent.Parent.Log) +local invariant = require(script.Parent.Parent.invariant) +local getProperty = require(script.Parent.getProperty) +local Error = require(script.Parent.Error) +local decodeValue = require(script.Parent.decodeValue) + +local function isEmpty(table) + return next(table) == nil +end + +local function shouldDeleteUnknownInstances(virtualInstance) + if virtualInstance.Metadata ~= nil then + return not virtualInstance.Metadata.ignoreUnknownInstances + else + return true + end +end + +local function diff(instanceMap, virtualInstances, rootId) + local patch = { + removed = {}, + added = {}, + updated = {}, + } + + -- Add a virtual instance and all of its descendants to the patch, marked as + -- being added. + local function markIdAdded(id) + local virtualInstance = virtualInstances[id] + patch.added[id] = virtualInstance + + for _, childId in ipairs(virtualInstance.Children) do + markIdAdded(childId) + end + end + + -- Internal recursive kernel for diffing an instance with the given ID. + local function diffInternal(id) + local virtualInstance = virtualInstances[id] + local instance = instanceMap.fromIds[id] + + if virtualInstance == nil then + invariant("Cannot diff an instance not present in virtualInstances\nID: {}", id) + end + + if instance == nil then + invariant("Cannot diff an instance not present in InstanceMap\nID: {}", id) + end + + if virtualInstance.ClassName ~= instance.ClassName then + error("unimplemented: support changing ClassName") + end + + local changedName = nil + if virtualInstance.Name ~= instance.Name then + changedName = virtualInstance.Name + end + + local changedProperties = {} + for propertyName, virtualValue in pairs(virtualInstance.Properties) do + local ok, existingValueOrErr = getProperty(instance, propertyName) + + if ok then + local existingValue = existingValueOrErr + local ok, decodedValue = decodeValue(virtualValue, instanceMap) + + if ok then + if existingValue ~= decodedValue then + changedProperties[propertyName] = virtualValue + end + else + Log.warn("Failed to decode property of type {}", virtualValue.Type) + end + else + local err = existingValueOrErr + + if err.kind == Error.UnknownProperty then + Log.trace("Skipping unknown property {}.{}", err.details.className, err.details.propertyName) + elseif err.kind == Error.UnreadableProperty then + Log.trace("Skipping unreadable property {}.{}", err.details.className, err.details.propertyName) + else + return false, err + end + end + end + + if changedName ~= nil or not isEmpty(changedProperties) then + table.insert(patch.updated, { + id = id, + changedName = changedName, + changedClassName = nil, + changedProperties = changedProperties, + changedMetadata = nil, + }) + end + + -- Traverse the list of children in the DOM. Any instance that has no + -- corresponding virtual instance should be removed. Any instance that + -- does have a corresponding virtual instance is recursively diffed. + for _, childInstance in ipairs(instance:GetChildren()) do + local childId = instanceMap.fromInstances[childInstance] + + if childId == nil then + -- This is an existing instance not present in the virtual DOM. + -- We can mark it for deletion unless the user has asked us not + -- to delete unknown stuff. + if shouldDeleteUnknownInstances(virtualInstance) then + table.insert(patch.removed, childInstance) + end + else + local ok, err = diffInternal(childId) + + if not ok then + return false, err + end + end + end + + -- Traverse the list of children in the virtual DOM. Any virtual + -- instance that has no corresponding real instance should be created. + for _, childId in ipairs(virtualInstance.Children) do + local childInstance = instanceMap.fromIds[childId] + + if childInstance == nil then + -- This instance is present in the virtual DOM, but doesn't + -- exist in the real DOM. + markIdAdded(childId) + end + end + + return true + end + + local ok, err = diffInternal(rootId) + + if not ok then + return false, err + end + + return true, patch +end + +return diff \ No newline at end of file diff --git a/plugin/src/Reconciler/diff.spec.lua b/plugin/src/Reconciler/diff.spec.lua new file mode 100644 index 00000000..47709554 --- /dev/null +++ b/plugin/src/Reconciler/diff.spec.lua @@ -0,0 +1,292 @@ +return function() + local Log = require(script.Parent.Parent.Parent.Log) + + local InstanceMap = require(script.Parent.Parent.InstanceMap) + local PatchSet = require(script.Parent.Parent.PatchSet) + + local diff = require(script.Parent.diff) + + local function isEmpty(table) + return next(table) == nil, "Table was not empty" + end + + local function size(dict) + local len = 0 + + for _ in pairs(dict) do + len = len + 1 + end + + return len + end + + it("should generate an empty patch for empty instances", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Some Name", + Properties = {}, + Children = {}, + }, + } + + local rootInstance = Instance.new("Folder") + rootInstance.Name = "Some Name" + knownInstances:insert("ROOT", rootInstance) + + local ok, patch = diff(knownInstances, virtualInstances, "ROOT") + + assert(ok, tostring(patch)) + + assert(isEmpty(patch.removed)) + assert(isEmpty(patch.added)) + assert(isEmpty(patch.updated)) + end) + + it("should generate a patch with a changed name", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Some Name", + Properties = {}, + Children = {}, + }, + } + + local rootInstance = Instance.new("Folder") + knownInstances:insert("ROOT", rootInstance) + + local ok, patch = diff(knownInstances, virtualInstances, "ROOT") + + assert(ok, tostring(patch)) + + assert(isEmpty(patch.removed)) + assert(isEmpty(patch.added)) + expect(#patch.updated).to.equal(1) + + local update = patch.updated[1] + expect(update.id).to.equal("ROOT") + expect(update.changedName).to.equal("Some Name") + assert(isEmpty(update.changedProperties)) + end) + + it("should generate a patch with a changed property", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "StringValue", + Name = "Value", + Properties = { + Value = { + Type = "String", + Value = "Hello, world!", + }, + }, + Children = {}, + }, + } + + local rootInstance = Instance.new("StringValue") + rootInstance.Value = "Initial Value" + knownInstances:insert("ROOT", rootInstance) + + local ok, patch = diff(knownInstances, virtualInstances, "ROOT") + + assert(ok, tostring(patch)) + + assert(isEmpty(patch.removed)) + assert(isEmpty(patch.added)) + expect(#patch.updated).to.equal(1) + + local update = patch.updated[1] + expect(update.id).to.equal("ROOT") + expect(update.changedName).to.equal(nil) + expect(size(update.changedProperties)).to.equal(1) + + local patchProperty = update.changedProperties["Value"] + expect(patchProperty).to.be.a("table") + expect(patchProperty.Type).to.equal("String") + expect(patchProperty.Value).to.equal("Hello, world!") + end) + + it("should generate an empty patch if no properties changed", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "StringValue", + Name = "Value", + Properties = { + Value = { + Type = "String", + Value = "Hello, world!", + }, + }, + Children = {}, + }, + } + + local rootInstance = Instance.new("StringValue") + rootInstance.Value = "Hello, world!" + knownInstances:insert("ROOT", rootInstance) + + local ok, patch = diff(knownInstances, virtualInstances, "ROOT") + + assert(ok, tostring(patch)) + assert(PatchSet.isEmpty(patch), "expected empty patch") + end) + + it("should ignore unknown properties", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Folder", + Properties = { + FAKE_PROPERTY = { + Type = "String", + Value = "Hello, world!", + }, + }, + Children = {}, + }, + } + + local rootInstance = Instance.new("Folder") + knownInstances:insert("ROOT", rootInstance) + + local ok, patch = diff(knownInstances, virtualInstances, "ROOT") + + assert(ok, tostring(patch)) + + assert(isEmpty(patch.removed)) + assert(isEmpty(patch.added)) + assert(isEmpty(patch.updated)) + end) + + --[[ + Because rbx_dom_lua resolves non-canonical properties to their canonical + variants, this test does not work as intended. + + Instead, heat_xml is diffed with Heat, the canonical property variant, + and a patch trying to assign to heat_xml is generated. This is + incorrect, but will require more invasive changes to fix later. + ]] + itFIXME("should ignore unreadable properties", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Fire", + Name = "Fire", + Properties = { + -- heat_xml is a serialization-only property that is not + -- exposed to Lua. + heat_xml = { + Type = "Float32", + Value = 5, + }, + }, + Children = {}, + }, + } + + local rootInstance = Instance.new("Fire") + knownInstances:insert("ROOT", rootInstance) + + local ok, patch = diff(knownInstances, virtualInstances, "ROOT") + + Log.warn("{:#?}", patch) + + assert(ok, tostring(patch)) + + assert(isEmpty(patch.removed)) + assert(isEmpty(patch.added)) + assert(isEmpty(patch.updated)) + end) + + it("should generate a patch removing unknown children by default", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Folder", + Properties = {}, + Children = {}, + }, + } + + local rootInstance = Instance.new("Folder") + local unknownChild = Instance.new("Folder") + unknownChild.Parent = rootInstance + knownInstances:insert("ROOT", rootInstance) + + local ok, patch = diff(knownInstances, virtualInstances, "ROOT") + + assert(ok, tostring(patch)) + + assert(isEmpty(patch.added)) + assert(isEmpty(patch.updated)) + expect(#patch.removed).to.equal(1) + expect(patch.removed[1]).to.equal(unknownChild) + end) + + it("should generate an empty patch if unknown children should be ignored", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Folder", + Properties = {}, + Children = {}, + Metadata = { + ignoreUnknownInstances = true, + }, + }, + } + + local rootInstance = Instance.new("Folder") + local unknownChild = Instance.new("Folder") + unknownChild.Parent = rootInstance + knownInstances:insert("ROOT", rootInstance) + + local ok, patch = diff(knownInstances, virtualInstances, "ROOT") + + assert(ok, tostring(patch)) + + assert(isEmpty(patch.added)) + assert(isEmpty(patch.updated)) + assert(isEmpty(patch.removed)) + end) + + it("should generate a patch with an added child", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Folder", + Properties = {}, + Children = {"CHILD"}, + }, + + CHILD = { + ClassName = "Folder", + Name = "Child", + Properties = {}, + Children = {}, + }, + } + + local rootInstance = Instance.new("Folder") + knownInstances:insert("ROOT", rootInstance) + + local ok, patch = diff(knownInstances, virtualInstances, "ROOT") + + assert(ok, tostring(patch)) + + assert(isEmpty(patch.updated)) + assert(isEmpty(patch.removed)) + expect(size(patch.added)).to.equal(1) + expect(patch.added["CHILD"]).to.equal(virtualInstances["CHILD"]) + end) +end \ No newline at end of file diff --git a/plugin/src/getCanonicalProperty.lua b/plugin/src/Reconciler/getProperty.lua similarity index 55% rename from plugin/src/getCanonicalProperty.lua rename to plugin/src/Reconciler/getProperty.lua index f594722a..bc67525c 100644 --- a/plugin/src/getCanonicalProperty.lua +++ b/plugin/src/Reconciler/getProperty.lua @@ -1,9 +1,11 @@ -local RbxDom = require(script.Parent.Parent.RbxDom) - --[[ Attempts to read a property from the given instance. ]] -local function getCanonincalProperty(instance, propertyName) + +local RbxDom = require(script.Parent.Parent.Parent.RbxDom) +local Error = require(script.Parent.Error) + +local function getProperty(instance, propertyName) local descriptor = RbxDom.findCanonicalPropertyDescriptor(instance.ClassName, propertyName) -- We can skip unknown properties; they're not likely reflected to Lua. @@ -11,11 +13,17 @@ local function getCanonincalProperty(instance, propertyName) -- A good example of a property like this is `Model.ModelInPrimary`, which -- is serialized but not reflected to Lua. if descriptor == nil then - return false, "unknown property" + return false, Error.new(Error.UnknownProperty, { + className = instance.ClassName, + propertyName = propertyName, + }) end if descriptor.scriptability == "None" or descriptor.scriptability == "Write" then - return false, "unreadable property" + return false, Error.new(Error.UnreadableProperty, { + className = instance.ClassName, + propertyName = propertyName, + }) end local success, valueOrErr = descriptor:read(instance) @@ -26,14 +34,19 @@ local function getCanonincalProperty(instance, propertyName) -- If we don't have permission to read a property, we can chalk that up -- to our database being out of date and the engine being right. if err.kind == RbxDom.Error.Kind.Roblox and err.extra:find("lacking permission") then - return false, "permission error" + return false, Error.new(Error.LackingPropertyPermissions, { + className = instance.ClassName, + propertyName = propertyName, + }) end - local message = ("Invalid property %s.%s: %s"):format(descriptor.className, descriptor.name, tostring(err)) - error(message, 2) + return false, Error.new(Error.OtherPropertyError, { + className = instance.ClassName, + propertyName = propertyName, + }) end return true, valueOrErr end -return getCanonincalProperty \ No newline at end of file +return getProperty \ No newline at end of file diff --git a/plugin/src/Reconciler/hydrate.lua b/plugin/src/Reconciler/hydrate.lua new file mode 100644 index 00000000..4d677f57 --- /dev/null +++ b/plugin/src/Reconciler/hydrate.lua @@ -0,0 +1,50 @@ +--[[ + Defines the process of "hydration" -- matching up a virtual DOM with + concrete instances and assigning them IDs. +]] + +local invariant = require(script.Parent.Parent.invariant) + +local function hydrate(instanceMap, virtualInstances, rootId, rootInstance) + local virtualInstance = virtualInstances[rootId] + + if virtualInstance == nil then + invariant("Cannot hydrate an instance not present in virtualInstances\nID: {}", rootId) + end + + instanceMap:insert(rootId, rootInstance) + + local existingChildren = rootInstance:GetChildren() + + -- For each existing child, we'll track whether it's been paired with an + -- instance that the Rojo server knows about. + local isExistingChildVisited = {} + for i = 1, #existingChildren do + isExistingChildVisited[i] = false + end + + for _, childId in ipairs(virtualInstance.Children) do + local virtualChild = virtualInstances[childId] + + for childIndex, childInstance in ipairs(existingChildren) do + if not isExistingChildVisited[childIndex] then + -- We guard accessing Name and ClassName in order to avoid + -- tripping over children of DataModel that Rojo won't have + -- permissions to access at all. + local ok, name, className = pcall(function() + return childInstance.Name, childInstance.ClassName + end) + + -- This rule is very conservative and could be loosened in the + -- future, or more heuristics could be introduced. + if ok and name == virtualChild.Name and className == virtualChild.ClassName then + isExistingChildVisited[childIndex] = true + hydrate(instanceMap, virtualInstances, childId, childInstance) + break + end + end + end + end +end + +return hydrate \ No newline at end of file diff --git a/plugin/src/Reconciler/hydrate.spec.lua b/plugin/src/Reconciler/hydrate.spec.lua new file mode 100644 index 00000000..b9ff796b --- /dev/null +++ b/plugin/src/Reconciler/hydrate.spec.lua @@ -0,0 +1,129 @@ +return function() + local hydrate = require(script.Parent.hydrate) + + local InstanceMap = require(script.Parent.Parent.InstanceMap) + + it("should match the root instance no matter what", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Model", + Name = "Foo", + Properties = {}, + Children = {}, + }, + } + + local rootInstance = Instance.new("Folder") + + hydrate(knownInstances, virtualInstances, "ROOT", rootInstance) + + expect(knownInstances:size()).to.equal(1) + expect(knownInstances.fromIds["ROOT"]).to.equal(rootInstance) + end) + + it("should not match children with mismatched ClassName", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Root", + Properties = {}, + Children = {}, + }, + + CHILD = { + ClassName = "Folder", + Name = "Child", + Properties = {}, + Children = {}, + }, + } + + local rootInstance = Instance.new("Folder") + + -- ClassName of this instance is intentionally different + local child = Instance.new("Model") + child.Name = "Child" + child.Parent = rootInstance + + hydrate(knownInstances, virtualInstances, "ROOT", rootInstance) + + expect(knownInstances:size()).to.equal(1) + expect(knownInstances.fromIds["ROOT"]).to.equal(rootInstance) + end) + + it("should not match children with mismatched Name", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Root", + Properties = {}, + Children = {}, + }, + + CHILD = { + ClassName = "Folder", + Name = "Child", + Properties = {}, + Children = {}, + }, + } + + local rootInstance = Instance.new("Folder") + + -- Name of this instance is intentionally different + local child = Instance.new("Folder") + child.Name = "Not Child" + child.Parent = rootInstance + + hydrate(knownInstances, virtualInstances, "ROOT", rootInstance) + + expect(knownInstances:size()).to.equal(1) + expect(knownInstances.fromIds["ROOT"]).to.equal(rootInstance) + end) + + it("should pair instances with matching Name and ClassName", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Root", + Properties = {}, + Children = {"CHILD1", "CHILD2"}, + }, + + CHILD1 = { + ClassName = "Folder", + Name = "Child 1", + Properties = {}, + Children = {}, + }, + + CHILD2 = { + ClassName = "Model", + Name = "Child 2", + Properties = {}, + Children = {}, + }, + } + + local rootInstance = Instance.new("Folder") + + local child1 = Instance.new("Folder") + child1.Name = "Child 1" + child1.Parent = rootInstance + + local child2 = Instance.new("Model") + child2.Name = "Child 2" + child2.Parent = rootInstance + + hydrate(knownInstances, virtualInstances, "ROOT", rootInstance) + + expect(knownInstances:size()).to.equal(3) + expect(knownInstances.fromIds["ROOT"]).to.equal(rootInstance) + expect(knownInstances.fromIds["CHILD1"]).to.equal(child1) + expect(knownInstances.fromIds["CHILD2"]).to.equal(child2) + end) +end \ No newline at end of file diff --git a/plugin/src/Reconciler/init.lua b/plugin/src/Reconciler/init.lua new file mode 100644 index 00000000..1db295a4 --- /dev/null +++ b/plugin/src/Reconciler/init.lua @@ -0,0 +1,34 @@ +--[[ + This module defines the meat of the Rojo plugin and how it manages tracking + and mutating the Roblox DOM. +]] + +local applyPatch = require(script.applyPatch) +local hydrate = require(script.hydrate) +local diff = require(script.diff) + +local Reconciler = {} +Reconciler.__index = Reconciler + +function Reconciler.new(instanceMap) + local self = { + -- Tracks all of the instances known by the reconciler by ID. + __instanceMap = instanceMap, + } + + return setmetatable(self, Reconciler) +end + +function Reconciler:applyPatch(patch) + return applyPatch(self.__instanceMap, patch) +end + +function Reconciler:hydrate(virtualInstances, rootId, rootInstance) + return hydrate(self.__instanceMap, virtualInstances, rootId, rootInstance) +end + +function Reconciler:diff(virtualInstances, rootId) + return diff(self.__instanceMap, virtualInstances, rootId) +end + +return Reconciler \ No newline at end of file diff --git a/plugin/src/Reconciler/reify.lua b/plugin/src/Reconciler/reify.lua new file mode 100644 index 00000000..8fbee41b --- /dev/null +++ b/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 \ No newline at end of file diff --git a/plugin/src/Reconciler/reify.spec.lua b/plugin/src/Reconciler/reify.spec.lua new file mode 100644 index 00000000..74335ec5 --- /dev/null +++ b/plugin/src/Reconciler/reify.spec.lua @@ -0,0 +1,352 @@ +return function() + local reify = require(script.Parent.reify) + + local PatchSet = require(script.Parent.Parent.PatchSet) + local InstanceMap = require(script.Parent.Parent.InstanceMap) + local Error = require(script.Parent.Error) + + local function isEmpty(table) + return next(table) == nil, "Table was not empty" + end + + local function size(dict) + local len = 0 + + for _ in pairs(dict) do + len = len + 1 + end + + return len + end + + it("should throw when given a bogus ID", function() + expect(function() + reify(InstanceMap.new(), {}, "Hi, mom!", game) + end).to.throw() + end) + + it("should return an error when given bogus class names", function() + local virtualInstances = { + ROOT = { + ClassName = "Balogna", + Name = "Food", + Properties = {}, + Children = {}, + }, + } + + local instanceMap = InstanceMap.new() + local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT", nil) + + assert(instanceMap:size() == 0, "expected instanceMap to be empty") + + expect(size(unappliedPatch.added)).to.equal(1) + expect(unappliedPatch.added["ROOT"]).to.equal(virtualInstances["ROOT"]) + + assert(isEmpty(unappliedPatch.removed), "expected no removes") + assert(isEmpty(unappliedPatch.updated), "expected no updates") + end) + + it("should assign name and properties", function() + local virtualInstances = { + ROOT = { + ClassName = "StringValue", + Name = "Spaghetti", + Properties = { + Value = { + Type = "String", + Value = "Hello, world!", + }, + }, + Children = {}, + }, + } + + local instanceMap = InstanceMap.new() + local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT") + + assert(PatchSet.isEmpty(unappliedPatch), "expected remaining patch to be empty") + + local instance = instanceMap.fromIds["ROOT"] + expect(instance.ClassName).to.equal("StringValue") + expect(instance.Name).to.equal("Spaghetti") + expect(instance.Value).to.equal("Hello, world!") + + expect(instanceMap:size()).to.equal(1) + end) + + it("should construct children", function() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Parent", + Properties = {}, + Children = {"CHILD"}, + }, + + CHILD = { + ClassName = "Folder", + Name = "Child", + Properties = {}, + Children = {}, + }, + } + + local instanceMap = InstanceMap.new() + local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT") + + assert(PatchSet.isEmpty(unappliedPatch), "expected remaining patch to be empty") + + local instance = instanceMap.fromIds["ROOT"] + expect(instance.ClassName).to.equal("Folder") + expect(instance.Name).to.equal("Parent") + + local child = instance.Child + expect(child.ClassName).to.equal("Folder") + + expect(instanceMap:size()).to.equal(2) + end) + + it("should still construct parents if children fail", function() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Parent", + Properties = {}, + Children = {"CHILD"}, + }, + + CHILD = { + ClassName = "this ain't an Instance", + Name = "Child", + Properties = {}, + Children = {}, + }, + } + + local instanceMap = InstanceMap.new() + local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT") + + expect(size(unappliedPatch.added)).to.equal(1) + expect(unappliedPatch.added["CHILD"]).to.equal(virtualInstances["CHILD"]) + assert(isEmpty(unappliedPatch.updated), "expected no updates") + assert(isEmpty(unappliedPatch.removed), "expected no removes") + + local instance = instanceMap.fromIds["ROOT"] + expect(instance.ClassName).to.equal("Folder") + expect(instance.Name).to.equal("Parent") + + expect(#instance:GetChildren()).to.equal(0) + expect(instanceMap:size()).to.equal(1) + end) + + it("should fail gracefully when setting erroneous properties", function() + local virtualInstances = { + ROOT = { + ClassName = "StringValue", + Name = "Root", + Properties = { + Value = { + Type = "Vector3", + Value = {1, 2, 3}, + }, + }, + Children = {}, + }, + } + + local instanceMap = InstanceMap.new() + local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT") + + local instance = instanceMap.fromIds["ROOT"] + expect(instance.ClassName).to.equal("StringValue") + expect(instance.Name).to.equal("Root") + + assert(isEmpty(unappliedPatch.added), "expected no additions") + expect(#unappliedPatch.updated).to.equal(1) + assert(isEmpty(unappliedPatch.removed), "expected no removes") + + local update = unappliedPatch.updated[1] + expect(update.id).to.equal("ROOT") + expect(size(update.changedProperties)).to.equal(1) + + local property = update.changedProperties["Value"] + expect(property).to.equal(virtualInstances["ROOT"].Properties.Value) + end) + + -- This is the simplest ref case: ensure that setting a ref property that + -- points to an instance that was previously created as part of the same + -- reify operation works. + it("should apply properties containing refs to ancestors", function() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Root", + Properties = {}, + Children = {"CHILD"}, + }, + + CHILD = { + ClassName = "ObjectValue", + Name = "Child", + Properties = { + Value = { + Type = "Ref", + Value = "ROOT", + }, + }, + Children = {}, + }, + } + + local instanceMap = InstanceMap.new() + local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT") + + assert(PatchSet.isEmpty(unappliedPatch), "expected remaining patch to be empty") + + local root = instanceMap.fromIds["ROOT"] + local child = instanceMap.fromIds["CHILD"] + expect(child.Value).to.equal(root) + end) + + -- This is another simple case: apply a ref property that points to an + -- existing instance. In this test, that instance was created before the + -- reify operation started and is present in instanceMap. + it("should apply properties containing refs to previously-existing instances", function() + local virtualInstances = { + ROOT = { + ClassName = "ObjectValue", + Name = "Root", + Properties = { + Value = { + Type = "Ref", + Value = "EXISTING", + }, + }, + Children = {}, + }, + } + + local instanceMap = InstanceMap.new() + + local existing = Instance.new("Folder") + existing.Name = "Existing" + instanceMap:insert("EXISTING", existing) + + local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT") + + assert(PatchSet.isEmpty(unappliedPatch), "expected remaining patch to be empty") + + local root = instanceMap.fromIds["ROOT"] + expect(root.Value).to.equal(existing) + end) + + -- This is a tricky ref case: CHILD_A points to CHILD_B, but is constructed + -- first. Deferred ref application is required to implement this case + -- correctly. + it("should apply properties containing refs to later siblings correctly", function() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Root", + Properties = {}, + Children = {"CHILD_A", "CHILD_B"}, + }, + + CHILD_A = { + ClassName = "ObjectValue", + Name = "Child A", + Properties = { + Value = { + Type = "Ref", + Value = "Child B", + }, + }, + Children = {}, + }, + + CHILD_B = { + ClassName = "Folder", + Name = "Child B", + Properties = {}, + Children = {}, + }, + } + + local instanceMap = InstanceMap.new() + local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT") + + assert(PatchSet.isEmpty(unappliedPatch), "expected remaining patch to be empty") + + local childA = instanceMap.fromIds["CHILD_A"] + local childB = instanceMap.fromIds["CHILD_B"] + expect(childA.Value).to.equal(childB) + end) + + -- This is the classic case that calls for deferred ref application. In this + -- test, the root instance has a ref property that refers to its child. The + -- root is definitely constructed first. + -- + -- This is distinct from the sibling case in that the child will be + -- constructed as part of a recursive call before the parent has totally + -- finished. Given deferred refs, this should not fail, but it is a good + -- case to test. + it("should apply properties containing refs to later siblings correctly", function() + local virtualInstances = { + ROOT = { + ClassName = "ObjectValue", + Name = "Root", + Properties = { + Value = { + Type = "Ref", + Value = "CHILD", + }, + }, + Children = {"CHILD"}, + }, + + CHILD = { + ClassName = "Folder", + Name = "Child", + Properties = {}, + Children = {}, + }, + } + + local instanceMap = InstanceMap.new() + local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT") + + assert(PatchSet.isEmpty(unappliedPatch), "expected remaining patch to be empty") + + local root = instanceMap.fromIds["ROOT"] + local child = instanceMap.fromIds["CHILD"] + expect(root.Value).to.equal(child) + end) + + it("should return a partial patch when applying invalid refs", function() + local virtualInstances = { + ROOT = { + ClassName = "ObjectValue", + Name = "Root", + Properties = { + Value = { + Type = "Ref", + Value = "SORRY", + }, + }, + Children = {}, + }, + } + + local instanceMap = InstanceMap.new() + local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT") + + assert(not PatchSet.hasRemoves(unappliedPatch), "expected no removes") + assert(not PatchSet.hasAdditions(unappliedPatch), "expected no additions") + expect(#unappliedPatch.updated).to.equal(1) + + local update = unappliedPatch.updated[1] + expect(update.id).to.equal("ROOT") + expect(update.changedProperties.Value).to.equal(virtualInstances["ROOT"].Properties.Value) + end) +end \ No newline at end of file diff --git a/plugin/src/Reconciler/setProperty.lua b/plugin/src/Reconciler/setProperty.lua new file mode 100644 index 00000000..316f920e --- /dev/null +++ b/plugin/src/Reconciler/setProperty.lua @@ -0,0 +1,48 @@ +--[[ + Attempts to set a property on the given instance. +]] + +local RbxDom = require(script.Parent.Parent.Parent.RbxDom) +local Log = require(script.Parent.Parent.Parent.Log) +local Error = require(script.Parent.Error) + +local function setProperty(instance, propertyName, value) + local descriptor = RbxDom.findCanonicalPropertyDescriptor(instance.ClassName, propertyName) + + -- We can skip unknown properties; they're not likely reflected to Lua. + -- + -- A good example of a property like this is `Model.ModelInPrimary`, which + -- is serialized but not reflected to Lua. + if descriptor == nil then + Log.trace("Skipping unknown property {}.{}", instance.ClassName, propertyName) + + return true + end + + if descriptor.scriptability == "None" or descriptor.scriptability == "Read" then + return false, Error.new(Error.UnwritableProperty, { + className = instance.ClassName, + propertyName = propertyName, + }) + end + + local ok, err = descriptor:write(instance, value) + + if not ok then + if err.kind == RbxDom.Error.Kind.Roblox and err.extra:find("lacking permission") then + return false, Error.new(Error.LackingPropertyPermissions, { + className = instance.ClassName, + propertyName = propertyName, + }) + end + + return false, Error.new(Error.OtherPropertyError, { + className = instance.ClassName, + propertyName = propertyName, + }) + end + + return true +end + +return setProperty \ No newline at end of file diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index e527cc6d..4bbe726c 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -5,6 +5,7 @@ local Fmt = require(script.Parent.Parent.Fmt) local t = require(script.Parent.Parent.t) local InstanceMap = require(script.Parent.InstanceMap) +local PatchSet = require(script.Parent.PatchSet) local Reconciler = require(script.Parent.Reconciler) local strict = require(script.Parent.strict) @@ -214,22 +215,36 @@ function ServeSession:__initialSync(rootInstanceId) -- the tree defined in this response. self.__apiContext:setMessageCursor(readResponseBody.messageCursor) - Log.trace("Computing changes that plugin needs to make to catch up to server...") + -- For any instances that line up with the Rojo server's view, start + -- tracking them in the reconciler. + Log.trace("Matching existing Roblox instances to Rojo IDs") + self.__reconciler:hydrate(readResponseBody.instances, rootInstanceId, game) -- Calculate the initial patch to apply to the DataModel to catch us -- up to what Rojo thinks the place should look like. - local hydratePatch = self.__reconciler:hydrate( + Log.trace("Computing changes that plugin needs to make to catch up to server...") + local success, catchUpPatch = self.__reconciler:diff( readResponseBody.instances, rootInstanceId, game ) - Log.trace("Computed hydration patch: {:#?}", debugPatch(hydratePatch)) + if not success then + Log.error("Could not compute a diff to catch up to the Rojo server: {:#?}", catchUpPatch) + end + + Log.trace("Computed hydration patch: {:#?}", debugPatch(catchUpPatch)) -- TODO: Prompt user to notify them of this patch, since it's - -- effectively a conflict between the Rojo server and the client. + -- effectively a conflict between the Rojo server and the client. In + -- the future, we'll ask which changes the user wants to keep. - self.__reconciler:applyPatch(hydratePatch) + local unappliedPatch = self.__reconciler:applyPatch(catchUpPatch) + + if not PatchSet.isEmpty(unappliedPatch) then + Log.warn("Could not apply all changes requested by the Rojo server:\n{}", + PatchSet.humanSummary(self.__instanceMap, unappliedPatch)) + end end) end @@ -237,7 +252,12 @@ function ServeSession:__mainSyncLoop() return self.__apiContext:retrieveMessages() :andThen(function(messages) for _, message in ipairs(messages) do - self.__reconciler:applyPatch(message) + local unappliedPatch = self.__reconciler:applyPatch(message) + + if not PatchSet.isEmpty(unappliedPatch) then + Log.warn("Could not apply all changes requested by the Rojo server:\n{}", + PatchSet.humanSummary(self.__instanceMap, unappliedPatch)) + end end if self.__status ~= Status.Disconnected then diff --git a/plugin/src/setCanonicalProperty.lua b/plugin/src/setCanonicalProperty.lua deleted file mode 100644 index 9554d020..00000000 --- a/plugin/src/setCanonicalProperty.lua +++ /dev/null @@ -1,37 +0,0 @@ -local RbxDom = require(script.Parent.Parent.RbxDom) - ---[[ - Attempts to set a property on the given instance. -]] -local function setCanonicalProperty(instance, propertyName, value) - local descriptor = RbxDom.findCanonicalPropertyDescriptor(instance.ClassName, propertyName) - - -- We can skip unknown properties; they're not likely reflected to Lua. - -- - -- A good example of a property like this is `Model.ModelInPrimary`, which - -- is serialized but not reflected to Lua. - if descriptor == nil then - return false, "unknown property" - end - - if descriptor.scriptability == "None" or descriptor.scriptability == "Read" then - return false, "unwritable property" - end - - local success, err = descriptor:write(instance, value) - - if not success then - -- If we don't have permission to write a property, we just silently - -- ignore it. - if err.kind == RbxDom.Error.Kind.Roblox and err.extra:find("lacking permission") then - return false, "permission error" - end - - local message = ("Invalid property %s.%s: %s"):format(descriptor.className, descriptor.name, tostring(err)) - error(message, 2) - end - - return true -end - -return setCanonicalProperty \ No newline at end of file diff --git a/plugin/test b/plugin/test new file mode 100644 index 00000000..caeb8874 --- /dev/null +++ b/plugin/test @@ -0,0 +1,4 @@ +#!/bin/sh + +rojo build test-place.project.json -o TestPlace.rbxlx +run-in-roblox --script run-tests.server.lua --place TestPlace.rbxlx \ No newline at end of file diff --git a/plugin/test-place.project.json b/plugin/test-place.project.json index 32901b26..7bc42eb6 100644 --- a/plugin/test-place.project.json +++ b/plugin/test-place.project.json @@ -15,7 +15,7 @@ "ServerScriptService": { "RunTests": { - "$path": "testBootstrap.server.lua" + "$path": "run-tests.server.lua" } }, diff --git a/selene.toml b/selene.toml new file mode 100644 index 00000000..e73e3567 --- /dev/null +++ b/selene.toml @@ -0,0 +1,4 @@ +std = "roblox" + +[config] +unused_variable = { allow_unused_self = true }