From bd2ea42732fcfb705c5a75ba098d9c2f7d1dd7e4 Mon Sep 17 00:00:00 2001 From: Cameron Campbell Date: Fri, 18 Apr 2025 16:44:11 +0100 Subject: [PATCH] Fixes issues with refs in the plugin. (#1005) --- .gitignore | 4 ++ plugin/fmt/init.lua | 4 +- plugin/http/Response.lua | 2 +- plugin/http/init.spec.lua | 2 +- plugin/log/init.lua | 2 +- plugin/log/init.spec.lua | 2 +- plugin/src/Reconciler/applyPatch.lua | 24 +++++++++++- plugin/src/Reconciler/diff.lua | 19 +++++++++- plugin/src/Reconciler/reify.lua | 41 +++++++++----------- plugin/src/Reconciler/reify.spec.lua | 57 ++++++++++++++++++++++------ 10 files changed, 114 insertions(+), 43 deletions(-) diff --git a/.gitignore b/.gitignore index b1a5efe9..61bdf0b2 100644 --- a/.gitignore +++ b/.gitignore @@ -19,3 +19,7 @@ # Snapshot files from the 'insta' Rust crate **/*.snap.new + +# Macos file system junk +._* +.DS_STORE diff --git a/plugin/fmt/init.lua b/plugin/fmt/init.lua index 99c61c9f..7ef24d8e 100644 --- a/plugin/fmt/init.lua +++ b/plugin/fmt/init.lua @@ -70,7 +70,7 @@ local function debugImpl(buffer, value, extendedForm) elseif valueType == "table" then local valueMeta = getmetatable(value) - if valueMeta ~= nil and valueMeta.__fmtDebug ~= nil then + if valueMeta ~= nil and valueMeta.__fmtDebug ~= nil then -- This type implement's the metamethod we made up to line up with -- Rust's 'Debug' trait. @@ -242,4 +242,4 @@ return { debugOutputBuffer = debugOutputBuffer, fmt = fmt, debugify = debugify, -} \ No newline at end of file +} diff --git a/plugin/http/Response.lua b/plugin/http/Response.lua index ce4e1b6d..1230a1c9 100644 --- a/plugin/http/Response.lua +++ b/plugin/http/Response.lua @@ -31,4 +31,4 @@ function Response:json() return HttpService:JSONDecode(self.body) end -return Response \ No newline at end of file +return Response diff --git a/plugin/http/init.spec.lua b/plugin/http/init.spec.lua index ff7e8e3b..ce62f802 100644 --- a/plugin/http/init.spec.lua +++ b/plugin/http/init.spec.lua @@ -2,4 +2,4 @@ return function() it("should load", function() require(script.Parent) end) -end \ No newline at end of file +end diff --git a/plugin/log/init.lua b/plugin/log/init.lua index 97fa7e46..882be997 100644 --- a/plugin/log/init.lua +++ b/plugin/log/init.lua @@ -57,4 +57,4 @@ function Log.error(template, ...) error(Fmt.fmt(template, ...)) end -return Log \ No newline at end of file +return Log diff --git a/plugin/log/init.spec.lua b/plugin/log/init.spec.lua index ff7e8e3b..ce62f802 100644 --- a/plugin/log/init.spec.lua +++ b/plugin/log/init.spec.lua @@ -2,4 +2,4 @@ return function() it("should load", function() require(script.Parent) end) -end \ No newline at end of file +end diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index 05e7412a..6c475f2f 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -16,6 +16,7 @@ local invariant = require(script.Parent.Parent.invariant) local decodeValue = require(script.Parent.decodeValue) local reify = require(script.Parent.reify) +local reifyInstance, applyDeferredRefs = reify.reifyInstance, reify.applyDeferredRefs local setProperty = require(script.Parent.setProperty) local function applyPatch(instanceMap, patch) @@ -29,6 +30,11 @@ local function applyPatch(instanceMap, patch) -- Tracks any portions of the patch that could not be applied to the DOM. local unappliedPatch = PatchSet.newEmpty() + -- Contains a list of all of the ref properties that we'll need to assign. + -- It is imperative that refs are assigned after all instances are created + -- to ensure that referents can be mapped to instances correctly. + local deferredRefs = {} + for _, removedIdOrInstance in ipairs(patch.removed) do local removeInstanceSuccess = pcall(function() if Types.RbxId(removedIdOrInstance) then @@ -78,7 +84,7 @@ local function applyPatch(instanceMap, patch) ) end - local failedToReify = reify(instanceMap, patch.added, id, parentInstance) + local failedToReify = reifyInstance(deferredRefs, instanceMap, patch.added, id, parentInstance) if not PatchSet.isEmpty(failedToReify) then Log.debug("Failed to reify as part of applying a patch: {:#?}", failedToReify) @@ -143,7 +149,7 @@ local function applyPatch(instanceMap, patch) [update.id] = mockVirtualInstance, } - local failedToReify = reify(instanceMap, mockAdded, update.id, instance.Parent) + local failedToReify = reifyInstance(deferredRefs, instanceMap, mockAdded, update.id, instance.Parent) local newInstance = instanceMap.fromIds[update.id] @@ -206,6 +212,18 @@ local function applyPatch(instanceMap, patch) if update.changedProperties ~= nil then for propertyName, propertyValue in pairs(update.changedProperties) do + -- Because refs may refer to instances that we haven't constructed yet, + -- we defer applying any ref properties until all instances are created. + if next(propertyValue) == "Ref" then + table.insert(deferredRefs, { + id = update.id, + instance = instance, + propertyName = propertyName, + virtualValue = propertyValue, + }) + continue + end + local decodeSuccess, decodedValue = decodeValue(propertyValue, instanceMap) if not decodeSuccess then unappliedUpdate.changedProperties[propertyName] = propertyValue @@ -230,6 +248,8 @@ local function applyPatch(instanceMap, patch) ChangeHistoryService:FinishRecording(historyRecording, Enum.FinishRecordingOperation.Commit) end + applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch) + return unappliedPatch end diff --git a/plugin/src/Reconciler/diff.lua b/plugin/src/Reconciler/diff.lua index c87f899c..97760829 100644 --- a/plugin/src/Reconciler/diff.lua +++ b/plugin/src/Reconciler/diff.lua @@ -151,7 +151,24 @@ local function diff(instanceMap, virtualInstances, rootId) if getProperySuccess then local existingValue = existingValueOrErr - local decodeSuccess, decodedValue = decodeValue(virtualValue, instanceMap) + local decodeSuccess, decodedValue + + -- If `virtualValue` is a ref then instead of decoding it to an instance, + -- we change `existingValue` to be a ref. This is because `virtualValue` + -- may point to an Instance which doesn't exist yet and therefore + -- decoding it may throw an error. + if next(virtualValue) == "Ref" then + decodeSuccess, decodedValue = true, virtualValue + + if existingValue and typeof(existingValue) == "Instance" then + local existingValueRef = instanceMap.fromInstances[existingValue] + if existingValueRef then + existingValue = { Ref = existingValueRef } + end + end + else + decodeSuccess, decodedValue = decodeValue(virtualValue, instanceMap) + end if decodeSuccess then if not trueEquals(existingValue, decodedValue) then diff --git a/plugin/src/Reconciler/reify.lua b/plugin/src/Reconciler/reify.lua index 9fbe4c9c..586dd342 100644 --- a/plugin/src/Reconciler/reify.lua +++ b/plugin/src/Reconciler/reify.lua @@ -7,26 +7,6 @@ local PatchSet = require(script.Parent.Parent.PatchSet) local setProperty = require(script.Parent.setProperty) local decodeValue = require(script.Parent.decodeValue) -local reifyInner, applyDeferredRefs - -local function reify(instanceMap, virtualInstances, rootId, parentInstance) - -- Create an empty patch that will be populated with any parts of this reify - -- that could not happen, like instances that couldn't be created and - -- properties that could not be assigned. - local unappliedPatch = PatchSet.newEmpty() - - -- Contains a list of all of the ref properties that we'll need to assign - -- after all instances are created. We apply refs in a second pass, after - -- we create as many instances as we can, so that we ensure that referents - -- can be mapped to instances correctly. - local deferredRefs = {} - - reifyInner(instanceMap, virtualInstances, rootId, parentInstance, unappliedPatch, deferredRefs) - applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch) - - return unappliedPatch -end - --[[ Add the given ID and all of its descendants in virtualInstances to the given PatchSet, marked for addition. @@ -40,10 +20,21 @@ local function addAllToPatch(patchSet, virtualInstances, id) end end +function reifyInstance(deferredRefs, instanceMap, virtualInstances, rootId, parentInstance) + -- Create an empty patch that will be populated with any parts of this reify + -- that could not happen, like instances that couldn't be created and + -- properties that could not be assigned. + local unappliedPatch = PatchSet.newEmpty() + + reifyInstanceInner(unappliedPatch, deferredRefs, instanceMap, virtualInstances, rootId, parentInstance) + + return unappliedPatch +end + --[[ Inner function that defines the core routine. ]] -function reifyInner(instanceMap, virtualInstances, id, parentInstance, unappliedPatch, deferredRefs) +function reifyInstanceInner(unappliedPatch, deferredRefs, instanceMap, virtualInstances, id, parentInstance) local virtualInstance = virtualInstances[id] if virtualInstance == nil then @@ -102,7 +93,7 @@ function reifyInner(instanceMap, virtualInstances, id, parentInstance, unapplied end for _, childId in ipairs(virtualInstance.Children) do - reifyInner(instanceMap, virtualInstances, childId, instance, unappliedPatch, deferredRefs) + reifyInstanceInner(unappliedPatch, deferredRefs, instanceMap, virtualInstances, childId, instance) end instance.Parent = parentInstance @@ -143,6 +134,7 @@ function applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch) end local targetInstance = instanceMap.fromIds[refId] + if targetInstance == nil then markFailed(entry.id, entry.propertyName, entry.virtualValue) continue @@ -155,4 +147,7 @@ function applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch) end end -return reify +return { + reifyInstance = reifyInstance, + applyDeferredRefs = applyDeferredRefs, +} diff --git a/plugin/src/Reconciler/reify.spec.lua b/plugin/src/Reconciler/reify.spec.lua index 44601dbc..3c8750fa 100644 --- a/plugin/src/Reconciler/reify.spec.lua +++ b/plugin/src/Reconciler/reify.spec.lua @@ -1,5 +1,6 @@ return function() local reify = require(script.Parent.reify) + local reifyInstance, applyDeferredRefs = reify.reifyInstance, reify.applyDeferredRefs local PatchSet = require(script.Parent.Parent.PatchSet) local InstanceMap = require(script.Parent.Parent.InstanceMap) @@ -20,7 +21,11 @@ return function() it("should throw when given a bogus ID", function() expect(function() - reify(InstanceMap.new(), {}, "Hi, mom!", game) + local deferredRefs = {} + local instanceMap = InstanceMap.new() + local unappliedPatch = reifyInstance(deferredRefs, instanceMap, {}, "Hi, mom!", game) + + applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch) end).to.throw() end) @@ -34,8 +39,11 @@ return function() }, } + local deferredRefs = {} local instanceMap = InstanceMap.new() - local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT", nil) + local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT", nil) + + applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch) assert(instanceMap:size() == 0, "expected instanceMap to be empty") @@ -60,8 +68,11 @@ return function() }, } + local deferredRefs = {} local instanceMap = InstanceMap.new() - local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT") + local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT") + + applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch) assert(PatchSet.isEmpty(unappliedPatch), "expected remaining patch to be empty") @@ -90,8 +101,11 @@ return function() }, } + local deferredRefs = {} local instanceMap = InstanceMap.new() - local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT") + local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT") + + applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch) assert(PatchSet.isEmpty(unappliedPatch), "expected remaining patch to be empty") @@ -122,8 +136,11 @@ return function() }, } + local deferredRefs = {} local instanceMap = InstanceMap.new() - local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT") + local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT") + + applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch) expect(size(unappliedPatch.added)).to.equal(1) expect(unappliedPatch.added["CHILD"]).to.equal(virtualInstances["CHILD"]) @@ -153,8 +170,11 @@ return function() }, } + local deferredRefs = {} local instanceMap = InstanceMap.new() - local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT") + local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT") + + applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch) local instance = instanceMap.fromIds["ROOT"] expect(instance.ClassName).to.equal("StringValue") @@ -196,8 +216,11 @@ return function() }, } + local deferredRefs = {} local instanceMap = InstanceMap.new() - local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT") + local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT") + + applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch) assert(PatchSet.isEmpty(unappliedPatch), "expected remaining patch to be empty") @@ -223,13 +246,16 @@ return function() }, } + local deferredRefs = {} local instanceMap = InstanceMap.new() local existing = Instance.new("Folder") existing.Name = "Existing" instanceMap:insert("EXISTING", existing) - local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT") + local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT") + + applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch) assert(PatchSet.isEmpty(unappliedPatch), "expected remaining patch to be empty") @@ -268,8 +294,11 @@ return function() }, } + local deferredRefs = {} local instanceMap = InstanceMap.new() - local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT") + local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT") + + applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch) assert(PatchSet.isEmpty(unappliedPatch), "expected remaining patch to be empty") @@ -307,8 +336,11 @@ return function() }, } + local deferredRefs = {} local instanceMap = InstanceMap.new() - local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT") + local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT") + + applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch) assert(PatchSet.isEmpty(unappliedPatch), "expected remaining patch to be empty") @@ -332,8 +364,11 @@ return function() }, } + local deferredRefs = {} local instanceMap = InstanceMap.new() - local unappliedPatch = reify(instanceMap, virtualInstances, "ROOT") + local unappliedPatch = reifyInstance(deferredRefs, instanceMap, virtualInstances, "ROOT") + + applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch) assert(not PatchSet.hasRemoves(unappliedPatch), "expected no removes") assert(not PatchSet.hasAdditions(unappliedPatch), "expected no additions")