diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 17205587..28565bb9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -72,7 +72,7 @@ jobs: run: cargo build --locked --verbose lint: - name: Rustfmt, Clippy, & Stylua + name: Rustfmt, Clippy, Stylua, & Selene runs-on: ubuntu-latest steps: @@ -98,6 +98,9 @@ jobs: - name: Stylua run: stylua --check plugin/src + - name: Selene + run: selene plugin/src + - name: Rustfmt run: cargo fmt -- --check diff --git a/CHANGELOG.md b/CHANGELOG.md index 7542adc7..afb46084 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,12 @@ # Rojo Changelog ## Unreleased Changes +* Fixed Rojo plugin failing to connect when project contains certain unreadable properties ([#848]) +* Fixed various cases where patch visualizer would not display sync failures ([#845], [#844]) + +[#848]: https://github.com/rojo-rbx/rojo/pull/848 +[#845]: https://github.com/rojo-rbx/rojo/pull/845 +[#844]: https://github.com/rojo-rbx/rojo/pull/844 ## [7.4.0] - January 16, 2024 * Improved the visualization for array properties like Tags ([#829]) diff --git a/aftman.toml b/aftman.toml index 86ad20fb..5273b6d0 100644 --- a/aftman.toml +++ b/aftman.toml @@ -1,5 +1,5 @@ [tools] rojo = "rojo-rbx/rojo@7.3.0" -selene = "Kampfkarren/selene@0.25.0" +selene = "Kampfkarren/selene@0.26.1" stylua = "JohnnyMorganz/stylua@0.18.2" run-in-roblox = "rojo-rbx/run-in-roblox@0.3.0" diff --git a/plugin/src/ApiContext.lua b/plugin/src/ApiContext.lua index 01b7ec45..68d43095 100644 --- a/plugin/src/ApiContext.lua +++ b/plugin/src/ApiContext.lua @@ -185,10 +185,10 @@ function ApiContext:write(patch) body = Http.jsonEncode(body) - return Http.post(url, body):andThen(rejectFailedRequests):andThen(Http.Response.json):andThen(function(body) - Log.info("Write response: {:?}", body) + return Http.post(url, body):andThen(rejectFailedRequests):andThen(Http.Response.json):andThen(function(responseBody) + Log.info("Write response: {:?}", responseBody) - return body + return responseBody end) end diff --git a/plugin/src/App/Components/Dropdown.lua b/plugin/src/App/Components/Dropdown.lua index 4b761636..f6814970 100644 --- a/plugin/src/App/Components/Dropdown.lua +++ b/plugin/src/App/Components/Dropdown.lua @@ -167,7 +167,7 @@ function Dropdown:render() self.setContentSize(object.AbsoluteContentSize) end, }), - Roact.createFragment(optionButtons), + Options = Roact.createFragment(optionButtons), }), }) else nil, diff --git a/plugin/src/App/Components/PatchVisualizer/DisplayValue.lua b/plugin/src/App/Components/PatchVisualizer/DisplayValue.lua index a231b879..4fdc4b48 100644 --- a/plugin/src/App/Components/PatchVisualizer/DisplayValue.lua +++ b/plugin/src/App/Components/PatchVisualizer/DisplayValue.lua @@ -57,7 +57,7 @@ local function DisplayValue(props) -- We don't need to support mixed tables, so checking the first key is enough -- to determine if it's a simple array local out, i = table.create(#props.value), 0 - for k, v in props.value do + for _, v in props.value do i += 1 -- Wrap strings in quotes diff --git a/plugin/src/App/Components/PatchVisualizer/DomLabel.lua b/plugin/src/App/Components/PatchVisualizer/DomLabel.lua index 8495d712..8b9af1c3 100644 --- a/plugin/src/App/Components/PatchVisualizer/DomLabel.lua +++ b/plugin/src/App/Components/PatchVisualizer/DomLabel.lua @@ -97,21 +97,16 @@ function DomLabel:render() -- Line guides help indent depth remain readable local lineGuides = {} for i = 1, props.depth or 0 do - table.insert( - lineGuides, - e("Frame", { - Name = "Line_" .. i, - Size = UDim2.new(0, 2, 1, 2), - Position = UDim2.new(0, (20 * i) + 15, 0, -1), - BorderSizePixel = 0, - BackgroundTransparency = props.transparency, - BackgroundColor3 = theme.BorderedContainer.BorderColor, - }) - ) + lineGuides["Line_" .. i] = e("Frame", { + Size = UDim2.new(0, 2, 1, 2), + Position = UDim2.new(0, (20 * i) + 15, 0, -1), + BorderSizePixel = 0, + BackgroundTransparency = props.transparency, + BackgroundColor3 = theme.BorderedContainer.BorderColor, + }) end return e("Frame", { - Name = "Change", ClipsDescendants = true, BackgroundColor3 = if props.patchType then theme.Diff[props.patchType] else nil, BorderSizePixel = 0, diff --git a/plugin/src/App/Components/TextButton.lua b/plugin/src/App/Components/TextButton.lua index 2cfae5a2..bd643c79 100644 --- a/plugin/src/App/Components/TextButton.lua +++ b/plugin/src/App/Components/TextButton.lua @@ -42,7 +42,7 @@ end function TextButton:render() return Theme.with(function(theme) local textSize = - TextService:GetTextSize(self.props.text, 18, Enum.Font.GothamSemibold, Vector2.new(math.huge, math.huge)) + TextService:GetTextSize(self.props.text, 18, Enum.Font.GothamMedium, Vector2.new(math.huge, math.huge)) local style = self.props.style @@ -83,7 +83,7 @@ function TextButton:render() Text = e("TextLabel", { Text = self.props.text, - Font = Enum.Font.GothamSemibold, + Font = Enum.Font.GothamMedium, TextSize = 18, TextColor3 = bindingUtil.mapLerp(bindingEnabled, theme.Enabled.TextColor, theme.Disabled.TextColor), TextTransparency = self.props.transparency, diff --git a/plugin/src/App/StatusPages/Confirming.lua b/plugin/src/App/StatusPages/Confirming.lua index 457340bd..c412d634 100644 --- a/plugin/src/App/StatusPages/Confirming.lua +++ b/plugin/src/App/StatusPages/Confirming.lua @@ -1,5 +1,3 @@ -local TextService = game:GetService("TextService") - local Rojo = script:FindFirstAncestor("Rojo") local Plugin = Rojo.Plugin local Packages = Rojo.Packages diff --git a/plugin/src/App/Theme.lua b/plugin/src/App/Theme.lua index d6f9622b..4b38d620 100644 --- a/plugin/src/App/Theme.lua +++ b/plugin/src/App/Theme.lua @@ -19,7 +19,6 @@ local Rojo = script:FindFirstAncestor("Rojo") local Packages = Rojo.Packages local Roact = require(Packages.Roact) -local Log = require(Packages.Log) local strict = require(script.Parent.Parent.strict) diff --git a/plugin/src/InstanceMap.lua b/plugin/src/InstanceMap.lua index 91a1d4cd..b035d423 100644 --- a/plugin/src/InstanceMap.lua +++ b/plugin/src/InstanceMap.lua @@ -113,27 +113,29 @@ end function InstanceMap:destroyInstance(instance) local id = self.fromInstances[instance] + local descendants = instance:GetDescendants() + instance:Destroy() + + -- After the instance is successfully destroyed, + -- we can remove all the id mappings + if id ~= nil then self:removeId(id) end - for _, descendantInstance in ipairs(instance:GetDescendants()) do + for _, descendantInstance in descendants do self:removeInstance(descendantInstance) end - - instance:Destroy() end function InstanceMap:destroyId(id) local instance = self.fromIds[id] - self:removeId(id) - if instance ~= nil then - for _, descendantInstance in ipairs(instance:GetDescendants()) do - self:removeInstance(descendantInstance) - end - - instance:Destroy() + self:destroyInstance(instance) + else + -- There is no instance with this id, so we can just remove the id + -- without worrying about instance destruction + self:removeId(id) end end diff --git a/plugin/src/PatchTree.lua b/plugin/src/PatchTree.lua index e67dbcd0..e469e211 100644 --- a/plugin/src/PatchTree.lua +++ b/plugin/src/PatchTree.lua @@ -426,22 +426,71 @@ function PatchTree.updateMetadata(tree, patch, instanceMap, unappliedPatch) -- Update isWarning metadata for _, failedChange in unappliedPatch.updated do local node = tree:getNode(failedChange.id) - if node then - node.isWarning = true - Log.trace("Marked node as warning: {} {}", node.id, node.name) - - if node.changeList then - for _, change in node.changeList do - if failedChange.changedProperties[change[1]] then - Log.trace(" Marked property as warning: {}", change[1]) - if change[4] == nil then - change[4] = {} - end - change[4].isWarning = true - end - end - end + if not node then + continue end + + node.isWarning = true + Log.trace("Marked node as warning: {} {}", node.id, node.name) + + if not node.changeList then + continue + end + for _, change in node.changeList do + local property = change[1] + local propertyFailedToApply = if property == "Name" + then failedChange.changedName ~= nil -- Name is not in changedProperties, so it needs a special case + else failedChange.changedProperties[property] ~= nil + + if not propertyFailedToApply then + -- This change didn't fail, no need to mark + continue + end + if change[4] == nil then + change[4] = { isWarning = true } + else + change[4].isWarning = true + end + Log.trace(" Marked property as warning: {}.{}", node.name, property) + end + end + for failedAdditionId in unappliedPatch.added do + local node = tree:getNode(failedAdditionId) + if not node then + continue + end + + node.isWarning = true + Log.trace("Marked node as warning: {} {}", node.id, node.name) + + if not node.changeList then + continue + end + for _, change in node.changeList do + -- Failed addition means that all properties failed to be added + if change[4] == nil then + change[4] = { isWarning = true } + else + change[4].isWarning = true + end + Log.trace(" Marked property as warning: {}.{}", node.name, change[1]) + end + end + for _, failedRemovalIdOrInstance in unappliedPatch.removed do + local failedRemovalId = if Types.RbxId(failedRemovalIdOrInstance) + then failedRemovalIdOrInstance + else instanceMap.fromInstances[failedRemovalIdOrInstance] + if not failedRemovalId then + continue + end + + local node = tree:getNode(failedRemovalId) + if not node then + continue + end + + node.isWarning = true + Log.trace("Marked node as warning: {} {}", node.id, node.name) end -- Update if instances exist diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index 4faa7310..5ee6ed5e 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -25,10 +25,15 @@ local function applyPatch(instanceMap, patch) local unappliedPatch = PatchSet.newEmpty() for _, removedIdOrInstance in ipairs(patch.removed) do - if Types.RbxId(removedIdOrInstance) then - instanceMap:destroyId(removedIdOrInstance) - else - instanceMap:destroyInstance(removedIdOrInstance) + local removeInstanceSuccess = pcall(function() + if Types.RbxId(removedIdOrInstance) then + instanceMap:destroyId(removedIdOrInstance) + else + instanceMap:destroyInstance(removedIdOrInstance) + end + end) + if not removeInstanceSuccess then + table.insert(unappliedPatch.removed, removedIdOrInstance) end end @@ -170,7 +175,13 @@ local function applyPatch(instanceMap, patch) end if update.changedName ~= nil then - instance.Name = update.changedName + local setNameSuccess = pcall(function() + instance.Name = update.changedName + end) + if not setNameSuccess then + unappliedUpdate.changedName = update.changedName + partiallyApplied = true + end end if update.changedMetadata ~= nil then @@ -183,15 +194,15 @@ local function applyPatch(instanceMap, patch) if update.changedProperties ~= nil then for propertyName, propertyValue in pairs(update.changedProperties) do - local ok, decodedValue = decodeValue(propertyValue, instanceMap) - if not ok then + local decodeSuccess, decodedValue = decodeValue(propertyValue, instanceMap) + if not decodeSuccess then unappliedUpdate.changedProperties[propertyName] = propertyValue partiallyApplied = true continue end - local ok = setProperty(instance, propertyName, decodedValue) - if not ok then + local setPropertySuccess = setProperty(instance, propertyName, decodedValue) + if not setPropertySuccess then unappliedUpdate.changedProperties[propertyName] = propertyValue partiallyApplied = true end diff --git a/plugin/src/Reconciler/decodeValue.lua b/plugin/src/Reconciler/decodeValue.lua index cd67e77d..9a171761 100644 --- a/plugin/src/Reconciler/decodeValue.lua +++ b/plugin/src/Reconciler/decodeValue.lua @@ -27,9 +27,9 @@ local function decodeValue(encodedValue, instanceMap) end end - local ok, decodedValue = RbxDom.EncodedValue.decode(encodedValue) + local decodeSuccess, decodedValue = RbxDom.EncodedValue.decode(encodedValue) - if not ok then + if not decodeSuccess then return false, Error.new(Error.CannotDecodeValue, { encodedValue = encodedValue, diff --git a/plugin/src/Reconciler/diff.lua b/plugin/src/Reconciler/diff.lua index 0bc772aa..c87f899c 100644 --- a/plugin/src/Reconciler/diff.lua +++ b/plugin/src/Reconciler/diff.lua @@ -147,13 +147,13 @@ local function diff(instanceMap, virtualInstances, rootId) local changedProperties = {} for propertyName, virtualValue in pairs(virtualInstance.Properties) do - local ok, existingValueOrErr = getProperty(instance, propertyName) + local getProperySuccess, existingValueOrErr = getProperty(instance, propertyName) - if ok then + if getProperySuccess then local existingValue = existingValueOrErr - local ok, decodedValue = decodeValue(virtualValue, instanceMap) + local decodeSuccess, decodedValue = decodeValue(virtualValue, instanceMap) - if ok then + if decodeSuccess then if not trueEquals(existingValue, decodedValue) then Log.debug( "{}.{} changed from '{}' to '{}'", @@ -165,7 +165,6 @@ local function diff(instanceMap, virtualInstances, rootId) changedProperties[propertyName] = virtualValue end else - local propertyType = next(virtualValue) Log.warn( "Failed to decode property {}.{}. Encoded property was: {:#?}", virtualInstance.ClassName, @@ -178,10 +177,8 @@ local function diff(instanceMap, virtualInstances, rootId) 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 + Log.trace("Skipping unreadable property {}.{}", err.details.className, err.details.propertyName) end end end @@ -220,9 +217,9 @@ local function diff(instanceMap, virtualInstances, rootId) table.insert(patch.removed, childInstance) end else - local ok, err = diffInternal(childId) + local diffSuccess, err = diffInternal(childId) - if not ok then + if not diffSuccess then return false, err end end @@ -243,9 +240,9 @@ local function diff(instanceMap, virtualInstances, rootId) return true end - local ok, err = diffInternal(rootId) + local diffSuccess, err = diffInternal(rootId) - if not ok then + if not diffSuccess then return false, err end diff --git a/plugin/src/Reconciler/hydrate.lua b/plugin/src/Reconciler/hydrate.lua index c2239179..353e7366 100644 --- a/plugin/src/Reconciler/hydrate.lua +++ b/plugin/src/Reconciler/hydrate.lua @@ -31,13 +31,13 @@ local function hydrate(instanceMap, virtualInstances, rootId, rootInstance) -- 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() + local accessSuccess, 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 + if accessSuccess and name == virtualChild.Name and className == virtualChild.ClassName then isExistingChildVisited[childIndex] = true hydrate(instanceMap, virtualInstances, childId, childInstance) break diff --git a/plugin/src/Reconciler/reify.lua b/plugin/src/Reconciler/reify.lua index 3c396573..9fbe4c9c 100644 --- a/plugin/src/Reconciler/reify.lua +++ b/plugin/src/Reconciler/reify.lua @@ -53,9 +53,9 @@ function reifyInner(instanceMap, virtualInstances, id, parentInstance, unapplied -- 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) + local createSuccess, instance = pcall(Instance.new, virtualInstance.ClassName) - if not ok then + if not createSuccess then addAllToPatch(unappliedPatch, virtualInstances, id) return end @@ -80,14 +80,14 @@ function reifyInner(instanceMap, virtualInstances, id, parentInstance, unapplied continue end - local ok, value = decodeValue(virtualValue, instanceMap) - if not ok then + local decodeSuccess, value = decodeValue(virtualValue, instanceMap) + if not decodeSuccess then unappliedProperties[propertyName] = virtualValue continue end - local ok = setProperty(instance, propertyName, value) - if not ok then + local setPropertySuccess = setProperty(instance, propertyName, value) + if not setPropertySuccess then unappliedProperties[propertyName] = virtualValue end end @@ -148,8 +148,8 @@ function applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch) continue end - local ok = setProperty(entry.instance, entry.propertyName, targetInstance) - if not ok then + local setPropertySuccess = setProperty(entry.instance, entry.propertyName, targetInstance) + if not setPropertySuccess then markFailed(entry.id, entry.propertyName, entry.virtualValue) end end diff --git a/plugin/src/Reconciler/reify.spec.lua b/plugin/src/Reconciler/reify.spec.lua index f39032a2..44601dbc 100644 --- a/plugin/src/Reconciler/reify.spec.lua +++ b/plugin/src/Reconciler/reify.spec.lua @@ -3,7 +3,6 @@ return function() 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" diff --git a/plugin/src/Reconciler/setProperty.lua b/plugin/src/Reconciler/setProperty.lua index 35c8b59b..11df6b54 100644 --- a/plugin/src/Reconciler/setProperty.lua +++ b/plugin/src/Reconciler/setProperty.lua @@ -28,9 +28,9 @@ local function setProperty(instance, propertyName, value) }) end - local ok, err = descriptor:write(instance, value) + local writeSuccess, err = descriptor:write(instance, value) - if not ok then + if not writeSuccess then if err.kind == RbxDom.Error.Kind.Roblox and err.extra:find("lacking permission") then return false, Error.new(Error.LackingPropertyPermissions, { diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index 56a06939..82d8fd42 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -21,8 +21,8 @@ local Status = strict("Session.Status", { Disconnected = "Disconnected", }) -local function debugPatch(patch) - return Fmt.debugify(patch, function(patch, output) +local function debugPatch(object) + return Fmt.debugify(object, function(patch, output) output:writeLine("Patch {{") output:indent() @@ -197,7 +197,7 @@ function ServeSession:__onActiveScriptChanged(activeScript) local existingParent = activeScript.Parent activeScript.Parent = nil - for i = 1, 3 do + for _ = 1, 3 do RunService.Heartbeat:Wait() end @@ -251,7 +251,10 @@ function ServeSession:__initialSync(serverInfo) if userDecision == "Abort" then return Promise.reject("Aborted Rojo sync operation") - elseif userDecision == "Reject" and self.__twoWaySync then + elseif userDecision == "Reject" then + if not self.__twoWaySync then + return Promise.reject("Cannot reject sync operation without two-way sync enabled") + end -- The user wants their studio DOM to write back to their Rojo DOM -- so we will reverse the patch and send it back @@ -268,7 +271,7 @@ function ServeSession:__initialSync(serverInfo) table.insert(inversePatch.updated, update) end -- Add the removed instances back to Rojo - -- selene:allow(empty_if, unused_variable) + -- selene:allow(empty_if, unused_variable, empty_loop) for _, instance in catchUpPatch.removed do -- TODO: Generate ID for our instance and add it to inversePatch.added end @@ -277,7 +280,7 @@ function ServeSession:__initialSync(serverInfo) table.insert(inversePatch.removed, id) end - self.__apiContext:write(inversePatch) + return self.__apiContext:write(inversePatch) elseif userDecision == "Accept" then local unappliedPatch = self.__reconciler:applyPatch(catchUpPatch) @@ -287,6 +290,10 @@ function ServeSession:__initialSync(serverInfo) PatchSet.humanSummary(self.__instanceMap, unappliedPatch) ) end + + return Promise.resolve() + else + return Promise.reject("Invalid user decision: " .. userDecision) end end) end diff --git a/plugin/src/createSignal.lua b/plugin/src/createSignal.lua deleted file mode 100644 index f6a00ddf..00000000 --- a/plugin/src/createSignal.lua +++ /dev/null @@ -1,53 +0,0 @@ ---[[ - Create a new signal that can be connected to, disconnected from, and fired. - - Usage: - - local signal = createSignal() - local disconnect = signal:connect(function(...) - print("fired:", ...) - end) - - signal:fire("a", "b", "c") - disconnect() - - Avoids mutating listeners list directly to prevent iterator invalidation if - a listener is disconnected while the signal is firing. -]] -local function createSignal() - local listeners = {} - - local function connect(newListener) - local nextListeners = {} - for listener in pairs(listeners) do - nextListeners[listener] = true - end - - nextListeners[newListener] = true - listeners = nextListeners - - return function() - local nextListeners = {} - for listener in pairs(listeners) do - if listener ~= newListener then - nextListeners[listener] = true - end - end - - listeners = nextListeners - end - end - - local function fire(...) - for listener in pairs(listeners) do - listener(...) - end - end - - return { - connect = connect, - fire = fire, - } -end - -return createSignal