When a patch fails, import the Instance directly using GetObjects

.git-blame-ignore-revs

@@ -0,0 +1,2 @@
+# stylua formatting
+0f8e1625d572a5fe0f7b5c08653ff92cc837d346

plugin/src/ApiContext.lua

@@ -239,4 +239,23 @@ function ApiContext:open(id)
 	end)
 end
 
+function ApiContext:fetch(ids: { string })
+	local url = ("%s/api/fetch"):format(self.__baseUrl)
+	local requestBody = {
+		sessionId = self.__sessionId,
+		idList = ids,
+	}
+
+	return Http.post(url, Http.jsonEncode(requestBody))
+		:andThen(rejectFailedRequests)
+		:andThen(Http.Response.json)
+		:andThen(function(responseBody)
+			if responseBody.sessionId ~= self.__sessionId then
+				return Promise.reject("Server changed ID")
+			end
+
+			return responseBody
+		end)
+end
+
 return ApiContext

plugin/src/App/StatusPages/Settings/init.lua

@@ -155,14 +155,24 @@ function SettingsPage:render()
 				layoutOrder = 5,
 			}),
 
+			FetchOnPatchFail = e(Setting, {
+				id = "fetchOnPatchFail",
+				name = "Load Model On Patch Fail",
+				description = "Whenever a patch fails to fully apply, send a request to the server to have it "
+					.. "placed into the local file system for the plugin to load.",
+				locked = self.props.syncActive,
+				transparency = self.props.transparency,
+				layoutOrder = 6,
+			}),
+
 			OpenScriptsExternally = e(Setting, {
 				id = "openScriptsExternally",
 				name = "Open Scripts Externally",
 				description = "Attempt to open scripts in an external editor",
 				locked = self.props.syncActive,
 				experimental = true,
 				transparency = self.props.transparency,
-				layoutOrder = 6,
+				layoutOrder = 7,
 			}),
 
 			TwoWaySync = e(Setting, {
@@ -172,7 +182,7 @@ function SettingsPage:render()
 				locked = self.props.syncActive,
 				experimental = true,
 				transparency = self.props.transparency,
-				layoutOrder = 7,
+				layoutOrder = 8,
 			}),
 
 			LogLevel = e(Setting, {

plugin/src/App/init.lua

@@ -347,6 +347,7 @@ function App:startSession()
 	local sessionOptions = {
 		openScriptsExternally = Settings:get("openScriptsExternally"),
 		twoWaySync = Settings:get("twoWaySync"),
+		fetchOnPatchFail = Settings:get("fetchOnPatchFail"),
 	}
 
 	local baseUrl = if string.find(host, "^https?://")
@@ -358,6 +359,7 @@ function App:startSession()
 		apiContext = apiContext,
 		openScriptsExternally = sessionOptions.openScriptsExternally,
 		twoWaySync = sessionOptions.twoWaySync,
+		fetchOnPatchFail = sessionOptions.fetchOnPatchFail,
 	})
 
 	self.cleanupPrecommit = serveSession.__reconciler:hookPrecommit(function(patch, instanceMap)

plugin/src/Reconciler/applyPatch.lua

@@ -5,8 +5,6 @@
 	Patches can come from the server or be generated by the client.
 ]]
 
-local ChangeHistoryService = game:GetService("ChangeHistoryService")
-
 local Packages = script.Parent.Parent.Parent.Packages
 local Log = require(Packages.Log)
 
@@ -19,8 +17,6 @@ local reify = require(script.Parent.reify)
 local setProperty = require(script.Parent.setProperty)
 
 local function applyPatch(instanceMap, patch)
-	local patchTimestamp = DateTime.now():FormatLocalTime("LTS", "en-us")
-
 	-- Tracks any portions of the patch that could not be applied to the DOM.
 	local unappliedPatch = PatchSet.newEmpty()
 
@@ -203,8 +199,6 @@ local function applyPatch(instanceMap, patch)
 		end
 	end
 
-	ChangeHistoryService:SetWaypoint("Rojo: Patch " .. patchTimestamp)
-
 	return unappliedPatch
 end
 

plugin/src/Reconciler/fetchInstances.lua

@@ -0,0 +1,73 @@
+local Selection = game:GetService("Selection")
+
+local Rojo = script:FindFirstAncestor("Rojo")
+local invariant = require(script.Parent.Parent.invariant)
+
+local Log = require(Rojo.Packages.Log)
+
+local function fetchInstances(idList, instanceMap, apiContext)
+	return apiContext:fetch(idList):andThen(function(body: { sessionId: string, path: string })
+		-- The endpoint `api/fetech/idlist` returns a table that contains
+		-- `sessionId` and `path`. The value of `path` is the name of a
+		-- file in the Content folder that may be loaded via `GetObjects`.
+		local objects = game:GetObjects("rbxasset://" .. body.path)
+		-- `ReferentMap` is a folder that contains nothing but
+		-- ObjectValues which will be named after entries in `instanceMap`
+		-- and have their `Value` property point towards a new Instance
+		-- that it can be swapped out with. In turn, `reified` is a
+		-- container for all of the objects pointed to by those instances.
+		local map = objects[1]:FindFirstChild("ReferentMap")
+		local reified = objects[1]:FindFirstChild("Reified")
+		if map == nil then
+			invariant("The fetch endpoint returned a malformed folder: missing ReferentMap")
+		end
+		if reified == nil then
+			invariant("The fetch endpoint returned a malformed folder: missing Reified")
+		end
+
+		-- We want to preserve selection between replacements.
+		local selected = Selection:Get()
+		local selectedMap = {}
+		for i, inst in selected do
+			selectedMap[inst] = i
+		end
+
+		for _, entry in map:GetChildren() do
+			if entry:IsA("ObjectValue") then
+				local key, value = entry.Name, entry.Value
+				if value == nil or not value:IsDescendantOf(reified) then
+					invariant("ReferentMap contained entry {} that was parented to an outside source", key)
+				else
+					-- This could be a problem if Roblox ever supports
+					-- parallel access to the DataModel but right now,
+					-- there's no way this results in a data race.
+					local oldInstance: Instance = instanceMap.fromIds[key]
+					instanceMap:insert(key, value)
+					Log.trace("Swapping Instance {} out", key)
+
+					local oldParent = oldInstance.Parent
+					local children = oldInstance:GetChildren()
+					for _, child in children do
+						child.Parent = value
+					end
+					value.Parent = oldParent
+					if selectedMap[oldInstance] then
+						-- Swapping section like this preserves order
+						-- It might not matter, but it's also basically free
+						-- So we may as well.
+						selected[selectedMap[oldInstance]] = value
+					end
+
+					-- So long and thanks for all the fish :-)
+					oldInstance:Destroy()
+				end
+			else
+				invariant("ReferentMap entry `{}` was a `{}` and not an ObjectValue", entry.Name, entry.ClassName)
+			end
+		end
+
+		Selection:Set(selected)
+	end)
+end
+
+return fetchInstances

plugin/src/Reconciler/init.lua

@@ -2,21 +2,28 @@
 	This module defines the meat of the Rojo plugin and how it manages tracking
 	and mutating the Roblox DOM.
 ]]
+local ChangeHistoryService = game:GetService("ChangeHistoryService")
 
 local Packages = script.Parent.Parent.Packages
 local Log = require(Packages.Log)
 
+local PatchSet = require(script.Parent.PatchSet)
+
+local fetchInstances = require(script.fetchInstances)
 local applyPatch = require(script.applyPatch)
 local hydrate = require(script.hydrate)
 local diff = require(script.diff)
 
 local Reconciler = {}
 Reconciler.__index = Reconciler
 
-function Reconciler.new(instanceMap)
+function Reconciler.new(instanceMap, apiContext, fetchOnPatchFail: boolean)
 	local self = {
 		-- Tracks all of the instances known by the reconciler by ID.
 		__instanceMap = instanceMap,
+		-- An API context for sending requests back to the server
+		__apiContext = apiContext,
+		__fetchOnPatchFail = fetchOnPatchFail,
 		__precommitCallbacks = {},
 		__postcommitCallbacks = {},
 	}
@@ -64,8 +71,29 @@ function Reconciler:applyPatch(patch)
 		end
 	end
 
+	local patchTimestamp = DateTime.now():FormatLocalTime("LTS", "en-us")
+
 	local unappliedPatch = applyPatch(self.__instanceMap, patch)
 
+	if self.__fetchOnPatchFail then
+		-- TODO Is it worth doing this for additions that fail?
+		-- It seems unlikely that a valid Instance can't be made with `Instance.new`
+		-- but can be made using GetObjects
+		if PatchSet.hasUpdates(unappliedPatch) then
+			local idList = table.create(#unappliedPatch.updated)
+			for i, entry in unappliedPatch.updated do
+				idList[i] = entry.id
+			end
+			-- TODO this is destructive to any properties that are
+			-- overwritten by the user but not known to Rojo. We can probably
+			-- mitigate that by keeping tabs of any instances we need to swap.
+			fetchInstances(idList, self.__instanceMap, self.__apiContext)
+			table.clear(unappliedPatch.updated)
+		end
+	end
+
+	ChangeHistoryService:SetWaypoint("Rojo: Patch " .. patchTimestamp)
+
 	for _, callback in self.__postcommitCallbacks do
 		local success, err = pcall(callback, patch, self.__instanceMap, unappliedPatch)
 		if not success then

plugin/src/ServeSession.lua

@@ -52,6 +52,7 @@ local validateServeOptions = t.strictInterface({
 	apiContext = t.table,
 	openScriptsExternally = t.boolean,
 	twoWaySync = t.boolean,
+	fetchOnPatchFail = t.boolean,
 })
 
 function ServeSession.new(options)
@@ -73,7 +74,7 @@ function ServeSession.new(options)
 
 	local instanceMap = InstanceMap.new(onInstanceChanged)
 	local changeBatcher = ChangeBatcher.new(instanceMap, onChangesFlushed)
-	local reconciler = Reconciler.new(instanceMap)
+	local reconciler = Reconciler.new(instanceMap, options.apiContext, options.fetchOnPatchFail)
 
 	local connections = {}
 
@@ -91,6 +92,7 @@ function ServeSession.new(options)
 		__apiContext = options.apiContext,
 		__openScriptsExternally = options.openScriptsExternally,
 		__twoWaySync = options.twoWaySync,
+		__fetchOnPatchFail = options.fetchOnPatchFail,
 		__reconciler = reconciler,
 		__instanceMap = instanceMap,
 		__changeBatcher = changeBatcher,

plugin/src/Settings.lua

@@ -13,6 +13,7 @@ local defaultSettings = {
 	openScriptsExternally = false,
 	twoWaySync = false,
 	showNotifications = true,
+	fetchOnPatchFail = true,
 	syncReminder = true,
 	confirmationBehavior = "Initial",
 	largeChangesConfirmationThreshold = 5,