From a67aec4df42addd7cfc93a9351dea98ef99a9272 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Sat, 26 Aug 2023 18:08:05 -0700 Subject: [PATCH 01/33] Pass ServeSession to reify --- plugin/src/Reconciler/applyPatch.lua | 6 +++--- plugin/src/Reconciler/init.lua | 4 ++-- plugin/src/Reconciler/reify.lua | 2 +- plugin/src/ServeSession.lua | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index 4faa73105..c50ce63d9 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -18,7 +18,7 @@ local decodeValue = require(script.Parent.decodeValue) local reify = require(script.Parent.reify) local setProperty = require(script.Parent.setProperty) -local function applyPatch(instanceMap, patch) +local function applyPatch(instanceMap, patch, serveSession) local patchTimestamp = DateTime.now():FormatLocalTime("LTS", "en-us") -- Tracks any portions of the patch that could not be applied to the DOM. @@ -65,7 +65,7 @@ local function applyPatch(instanceMap, patch) ) end - local failedToReify = reify(instanceMap, patch.added, id, parentInstance) + local failedToReify = reify(instanceMap, patch.added, id, parentInstance, serveSession) if not PatchSet.isEmpty(failedToReify) then Log.debug("Failed to reify as part of applying a patch: {:#?}", failedToReify) @@ -130,7 +130,7 @@ local function applyPatch(instanceMap, patch) [update.id] = mockVirtualInstance, } - local failedToReify = reify(instanceMap, mockAdded, update.id, instance.Parent) + local failedToReify = reify(instanceMap, mockAdded, update.id, instance.Parent, serveSession) local newInstance = instanceMap.fromIds[update.id] diff --git a/plugin/src/Reconciler/init.lua b/plugin/src/Reconciler/init.lua index 129457827..2855f989a 100644 --- a/plugin/src/Reconciler/init.lua +++ b/plugin/src/Reconciler/init.lua @@ -56,7 +56,7 @@ function Reconciler:hookPostcommit(callback: (patch: any, instanceMap: any, unap end end -function Reconciler:applyPatch(patch) +function Reconciler:applyPatch(patch, serveSession) for _, callback in self.__precommitCallbacks do local success, err = pcall(callback, patch, self.__instanceMap) if not success then @@ -64,7 +64,7 @@ function Reconciler:applyPatch(patch) end end - local unappliedPatch = applyPatch(self.__instanceMap, patch) + local unappliedPatch = applyPatch(self.__instanceMap, patch, serveSession) for _, callback in self.__postcommitCallbacks do local success, err = pcall(callback, patch, self.__instanceMap, unappliedPatch) diff --git a/plugin/src/Reconciler/reify.lua b/plugin/src/Reconciler/reify.lua index 3c3965734..82ca57d9f 100644 --- a/plugin/src/Reconciler/reify.lua +++ b/plugin/src/Reconciler/reify.lua @@ -9,7 +9,7 @@ local decodeValue = require(script.Parent.decodeValue) local reifyInner, applyDeferredRefs -local function reify(instanceMap, virtualInstances, rootId, parentInstance) +local function reify(instanceMap, virtualInstances, rootId, parentInstance, serveSession) -- 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. diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index 6a3fcdc4d..4b6eb8426 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -288,7 +288,7 @@ function ServeSession:__initialSync(serverInfo) self.__apiContext:write(inversePatch) elseif userDecision == "Accept" then - local unappliedPatch = self.__reconciler:applyPatch(catchUpPatch) + local unappliedPatch = self.__reconciler:applyPatch(catchUpPatch, self) if not PatchSet.isEmpty(unappliedPatch) then Log.warn("Could not apply all changes requested by the Rojo server:\n{}", @@ -312,7 +312,7 @@ function ServeSession:__mainSyncLoop() Log.trace("Serve session {} retrieved {} messages", tostring(self), #messages) for _, message in messages do - local unappliedPatch = self.__reconciler:applyPatch(message) + local unappliedPatch = self.__reconciler:applyPatch(message, self) if not PatchSet.isEmpty(unappliedPatch) then Log.warn("Could not apply all changes requested by the Rojo server:\n{}", From 0017861c13741fc2df502401fc47d99d918b8f54 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Sun, 27 Aug 2023 20:05:20 -0700 Subject: [PATCH 02/33] Use tempfile as a normal dependency --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 4c2109341..ec972e5c2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -90,6 +90,7 @@ uuid = { version = "1.0.0", features = ["v4", "serde"] } clap = { version = "3.1.18", features = ["derive"] } profiling = "1.0.6" tracy-client = { version = "0.13.2", optional = true } +tempfile = "3.2.0" [target.'cfg(windows)'.dependencies] winreg = "0.10.1" @@ -111,5 +112,4 @@ insta = { version = "1.8.0", features = ["redactions", "yaml"] } paste = "1.0.5" pretty_assertions = "1.2.1" serde_yaml = "0.8.21" -tempfile = "3.2.0" walkdir = "2.3.2" From 70b18990a0cfefa964e4423bd2bdda37394c9714 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Sun, 27 Aug 2023 20:06:10 -0700 Subject: [PATCH 03/33] Store a lazy ref to a temp dir in ServeSession --- src/serve_session.rs | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/serve_session.rs b/src/serve_session.rs index 9dab5446e..a3a8acee7 100644 --- a/src/serve_session.rs +++ b/src/serve_session.rs @@ -4,13 +4,14 @@ use std::{ io, net::IpAddr, path::{Path, PathBuf}, - sync::{Arc, Mutex, MutexGuard}, + sync::{Arc, Mutex, MutexGuard, OnceLock}, time::Instant, }; use crossbeam_channel::Sender; use memofs::IoResultExt; use memofs::Vfs; +use tempfile::TempDir; use thiserror::Error; use crate::{ @@ -87,6 +88,10 @@ pub struct ServeSession { /// A channel to send mutation requests on. These will be handled by the /// ChangeProcessor and trigger changes in the tree. tree_mutation_sender: Sender, + + /// A temporary directory that is cleaned up at the end of this structure's + /// lifespan. This is not initialized directly to avoid unnecessary work. + temporary_folder: OnceLock>, } impl ServeSession { @@ -160,6 +165,7 @@ impl ServeSession { message_queue, tree_mutation_sender, vfs, + temporary_folder: OnceLock::new(), }) } @@ -219,6 +225,20 @@ impl ServeSession { pub fn root_dir(&self) -> &Path { self.root_project.folder_location() } + + /// Returns a path to a temporary directory if it could be created. + /// Otherwise, returns `None`. + pub fn temp_dir(&self) -> Option<&Path> { + self.temporary_folder + .get_or_init( + || match tempfile::Builder::new().prefix("rojo-").tempdir() { + Ok(dir) => Some(dir), + Err(_) => None, + }, + ) + .as_ref() + .map(|dir| dir.path()) + } } #[derive(Debug, Error)] From ef909261faf2f9b8c2a1bdfe6e7dbd9126a5538e Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Sun, 27 Aug 2023 20:07:40 -0700 Subject: [PATCH 04/33] Initial implementation of `fetch` endpoint --- src/web/api.rs | 122 +++++++++++++++++++++++++++++++++++++++++-- src/web/interface.rs | 8 +++ 2 files changed, 127 insertions(+), 3 deletions(-) diff --git a/src/web/api.rs b/src/web/api.rs index 338862f7c..0f68ab8c2 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -1,18 +1,23 @@ //! Defines Rojo's HTTP API, all under /api. These endpoints generally return //! JSON. -use std::{collections::HashMap, fs, path::PathBuf, str::FromStr, sync::Arc}; +use std::{collections::HashMap, fs, io::BufWriter, path::PathBuf, str::FromStr, sync::Arc}; use hyper::{body, Body, Method, Request, Response, StatusCode}; use opener::OpenError; -use rbx_dom_weak::types::Ref; +use rbx_dom_weak::{ + types::{Ref, Variant}, + InstanceBuilder, WeakDom, +}; +use roblox_install::RobloxStudio; +use uuid::Uuid; use crate::{ serve_session::ServeSession, snapshot::{InstanceWithMeta, PatchSet, PatchUpdate}, web::{ interface::{ - ErrorResponse, Instance, OpenResponse, ReadResponse, ServerInfoResponse, + ErrorResponse, FetchResponse, Instance, OpenResponse, ReadResponse, ServerInfoResponse, SubscribeMessage, SubscribeResponse, WriteRequest, WriteResponse, PROTOCOL_VERSION, SERVER_VERSION, }, @@ -23,6 +28,7 @@ use crate::{ pub async fn call(serve_session: Arc, request: Request) -> Response { let service = ApiService::new(serve_session); + log::debug!("{} request received to {}", request.method(), request.uri()); match (request.method(), request.uri().path()) { (&Method::GET, "/api/rojo") => service.handle_api_rojo().await, (&Method::GET, path) if path.starts_with("/api/read/") => { @@ -37,6 +43,10 @@ pub async fn call(serve_session: Arc, request: Request) -> R (&Method::POST, "/api/write") => service.handle_api_write(request).await, + (&Method::GET, path) if path.starts_with("/api/fetch/") => { + service.handle_api_fetch_get(request).await + } + (_method, path) => json( ErrorResponse::not_found(format!("Route not found: {}", path)), StatusCode::NOT_FOUND, @@ -275,6 +285,112 @@ impl ApiService { session_id: self.serve_session.session_id(), }) } + + async fn handle_api_fetch_get(&self, request: Request) -> Response { + let argument = &request.uri().path()["/api/fetch/".len()..]; + let requested_ids: Vec = match argument.split(',').map(Ref::from_str).collect() { + Ok(ids) => ids, + Err(_) => { + return json( + ErrorResponse::bad_request("Malformed ID list"), + StatusCode::BAD_REQUEST, + ); + } + }; + + let content_dir = match RobloxStudio::locate() { + Ok(path) => path.content_path().to_path_buf(), + Err(_) => { + return json( + ErrorResponse::internal_error("Cannot locate Studio install"), + StatusCode::INTERNAL_SERVER_ERROR, + ) + } + }; + + let temp_dir = match self.serve_session.temp_dir() { + Some(dir) => dir, + None => { + return json( + ErrorResponse::bad_request("could not create temporary directory"), + StatusCode::INTERNAL_SERVER_ERROR, + ) + } + }; + + let uuid = Uuid::new_v4(); + let mut file_name = PathBuf::from(uuid.to_string()); + file_name.set_extension("rbxm"); + + let out_path = temp_dir.join(&file_name); + let studio_path = content_dir.join(&file_name); + + let mut writer = BufWriter::new(match fs::File::create(&out_path) { + Ok(handle) => handle, + Err(_) => { + return json( + ErrorResponse::internal_error("Could not create temporary file"), + StatusCode::INTERNAL_SERVER_ERROR, + ); + } + }); + + let tree = self.serve_session.tree(); + let inner_tree = tree.inner(); + let mut sub_tree = WeakDom::new(InstanceBuilder::new("Folder")); + let reify_ref = sub_tree.insert( + sub_tree.root_ref(), + InstanceBuilder::new("Folder").with_name("Reified"), + ); + let map_ref = sub_tree.insert( + sub_tree.root_ref(), + InstanceBuilder::new("Folder").with_name("ReferentMap"), + ); + for referent in requested_ids { + if inner_tree.get_by_ref(referent).is_some() { + log::trace!("Creating clone of {referent} into subtree"); + let new_ref = inner_tree.clone_into_external(referent, &mut sub_tree); + sub_tree.transfer_within(new_ref, reify_ref); + sub_tree.insert( + map_ref, + InstanceBuilder::new("ObjectValue") + .with_property("Value", Variant::Ref(new_ref)) + .with_name(referent.to_string()), + ); + } else { + // TODO handle this better + log::error!("bad ref {referent}! ahh!") + } + } + if let Err(_) = rbx_binary::to_writer(&mut writer, &sub_tree, &[sub_tree.root_ref()]) { + return json( + ErrorResponse::internal_error("Could not build subtree into model file"), + StatusCode::INTERNAL_SERVER_ERROR, + ); + } + drop(tree); + + log::debug!("Wrote model file to {}", out_path.display()); + + match fs_err::hard_link(&out_path, &studio_path) { + Ok(_) => { + log::debug!("Created hardlink to {}", &studio_path.display()); + json_ok(FetchResponse { + session_id: self.serve_session.session_id(), + path: file_name.to_string_lossy(), + }) + } + Err(err) => { + log::debug!("Failed to create hardlink to {}", &studio_path.display()); + json( + ErrorResponse::internal_error(format!( + "Could not put file in Roblox content folder: {err}" + )), + StatusCode::SERVICE_UNAVAILABLE, + ) + } + } + } } /// If this instance is represented by a script, try to find the correct .lua or .luau diff --git a/src/web/interface.rs b/src/web/interface.rs index c10e14bc9..9444be644 100644 --- a/src/web/interface.rs +++ b/src/web/interface.rs @@ -204,6 +204,14 @@ pub struct OpenResponse { pub session_id: SessionId, } +/// Response body from GET /api/fetch/{ids} +#[derive(Debug, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FetchResponse<'a> { + pub session_id: SessionId, + pub path: Cow<'a, str>, +} + /// General response type returned from all Rojo routes #[derive(Debug, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] From a5af0e89162c2f89d73183e232098584a99d6ef3 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Sun, 27 Aug 2023 21:05:01 -0700 Subject: [PATCH 05/33] Add initial implementation of fetch api for client --- plugin/src/ApiContext.lua | 14 ++++++++ plugin/src/Reconciler/reify.lua | 58 ++++++++++++++++++++++++++++++++- 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/plugin/src/ApiContext.lua b/plugin/src/ApiContext.lua index 9a683dd55..fcb99919d 100644 --- a/plugin/src/ApiContext.lua +++ b/plugin/src/ApiContext.lua @@ -253,4 +253,18 @@ function ApiContext:open(id) end) end +function ApiContext:fetch(ids: {string}) + local url = ("%s/api/fetch/%s"):format(self.__baseUrl, table.concat(ids, ",")) + return Http.get(url) + :andThen(rejectFailedRequests) + :andThen(Http.Response.json) + :andThen(function(body) + if body.sessionId ~= self.__sessionId then + return Promise.reject("Server changed ID") + end + + return body + end) +end + return ApiContext diff --git a/plugin/src/Reconciler/reify.lua b/plugin/src/Reconciler/reify.lua index 82ca57d9f..10264c2ef 100644 --- a/plugin/src/Reconciler/reify.lua +++ b/plugin/src/Reconciler/reify.lua @@ -2,12 +2,16 @@ "Reifies" a virtual DOM, constructing a real DOM with the same shape. ]] +local Rojo = script:FindFirstAncestor("Rojo") + 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 Log = require(Rojo.Packages.Log) + +local reifyInner, fetchUnapplied, applyDeferredRefs local function reify(instanceMap, virtualInstances, rootId, parentInstance, serveSession) -- Create an empty patch that will be populated with any parts of this reify @@ -155,4 +159,56 @@ function applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch) end end +function fetchInstances(idList, instanceMap, serveSession) + return serveSession.__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`. + Log.debug("Loading assets for {}", idList) + 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 + for _, entry in map:GetChildren() do + if entry:IsA("ObjectValue") then + local key, value = entry.Name, entry.Value + if value == nil or value.Parent ~= 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.debug("Swapping Instance {} out", oldInstance:GetFullName()) + + local oldParent = oldInstance.Parent + local children = oldInstance:GetChildren() + for _, child in children do + child.Parent = value + end + value.Parent = oldParent + + -- 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 + + end) +end + return reify From 7145d70d24c5283b68e27381e5eaa26404e8d52f Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Sat, 16 Sep 2023 20:16:58 -0700 Subject: [PATCH 06/33] Don't actually pass ServeSession to reify this was widely regarded as a bad move --- plugin/src/Reconciler/applyPatch.lua | 6 +++--- plugin/src/Reconciler/init.lua | 4 ++-- plugin/src/Reconciler/reify.lua | 4 ++-- plugin/src/ServeSession.lua | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index c50ce63d9..4faa73105 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -18,7 +18,7 @@ local decodeValue = require(script.Parent.decodeValue) local reify = require(script.Parent.reify) local setProperty = require(script.Parent.setProperty) -local function applyPatch(instanceMap, patch, serveSession) +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. @@ -65,7 +65,7 @@ local function applyPatch(instanceMap, patch, serveSession) ) end - local failedToReify = reify(instanceMap, patch.added, id, parentInstance, serveSession) + 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) @@ -130,7 +130,7 @@ local function applyPatch(instanceMap, patch, serveSession) [update.id] = mockVirtualInstance, } - local failedToReify = reify(instanceMap, mockAdded, update.id, instance.Parent, serveSession) + local failedToReify = reify(instanceMap, mockAdded, update.id, instance.Parent) local newInstance = instanceMap.fromIds[update.id] diff --git a/plugin/src/Reconciler/init.lua b/plugin/src/Reconciler/init.lua index 2855f989a..129457827 100644 --- a/plugin/src/Reconciler/init.lua +++ b/plugin/src/Reconciler/init.lua @@ -56,7 +56,7 @@ function Reconciler:hookPostcommit(callback: (patch: any, instanceMap: any, unap end end -function Reconciler:applyPatch(patch, serveSession) +function Reconciler:applyPatch(patch) for _, callback in self.__precommitCallbacks do local success, err = pcall(callback, patch, self.__instanceMap) if not success then @@ -64,7 +64,7 @@ function Reconciler:applyPatch(patch, serveSession) end end - local unappliedPatch = applyPatch(self.__instanceMap, patch, serveSession) + local unappliedPatch = applyPatch(self.__instanceMap, patch) for _, callback in self.__postcommitCallbacks do local success, err = pcall(callback, patch, self.__instanceMap, unappliedPatch) diff --git a/plugin/src/Reconciler/reify.lua b/plugin/src/Reconciler/reify.lua index 10264c2ef..37a1583aa 100644 --- a/plugin/src/Reconciler/reify.lua +++ b/plugin/src/Reconciler/reify.lua @@ -11,9 +11,9 @@ local decodeValue = require(script.Parent.decodeValue) local Log = require(Rojo.Packages.Log) -local reifyInner, fetchUnapplied, applyDeferredRefs +local reifyInner, applyDeferredRefs -local function reify(instanceMap, virtualInstances, rootId, parentInstance, serveSession) +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. diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index 4b6eb8426..6a3fcdc4d 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -288,7 +288,7 @@ function ServeSession:__initialSync(serverInfo) self.__apiContext:write(inversePatch) elseif userDecision == "Accept" then - local unappliedPatch = self.__reconciler:applyPatch(catchUpPatch, self) + 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{}", @@ -312,7 +312,7 @@ function ServeSession:__mainSyncLoop() Log.trace("Serve session {} retrieved {} messages", tostring(self), #messages) for _, message in messages do - local unappliedPatch = self.__reconciler:applyPatch(message, self) + 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{}", From f2cdcae5ce989bf2867f89ccac231373b5a87459 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Sat, 16 Sep 2023 20:45:06 -0700 Subject: [PATCH 07/33] Pass apiContext to reconciler and applyPatch --- plugin/src/Reconciler/applyPatch.lua | 2 +- plugin/src/Reconciler/init.lua | 6 ++++-- plugin/src/ServeSession.lua | 4 ++-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index 4faa73105..b2aca7dd1 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -18,7 +18,7 @@ local decodeValue = require(script.Parent.decodeValue) local reify = require(script.Parent.reify) local setProperty = require(script.Parent.setProperty) -local function applyPatch(instanceMap, patch) +local function applyPatch(instanceMap, apiContext, patch) local patchTimestamp = DateTime.now():FormatLocalTime("LTS", "en-us") -- Tracks any portions of the patch that could not be applied to the DOM. diff --git a/plugin/src/Reconciler/init.lua b/plugin/src/Reconciler/init.lua index 129457827..cf491aed6 100644 --- a/plugin/src/Reconciler/init.lua +++ b/plugin/src/Reconciler/init.lua @@ -13,10 +13,12 @@ local diff = require(script.diff) local Reconciler = {} Reconciler.__index = Reconciler -function Reconciler.new(instanceMap) +function Reconciler.new(instanceMap, apiContext) 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, __precommitCallbacks = {}, __postcommitCallbacks = {}, } @@ -64,7 +66,7 @@ function Reconciler:applyPatch(patch) end end - local unappliedPatch = applyPatch(self.__instanceMap, patch) + local unappliedPatch = applyPatch(self.__instanceMap, self.__apiContext, patch) for _, callback in self.__postcommitCallbacks do local success, err = pcall(callback, patch, self.__instanceMap, unappliedPatch) diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index 6a3fcdc4d..160240ea6 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -73,7 +73,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) local connections = {} @@ -288,7 +288,7 @@ function ServeSession:__initialSync(serverInfo) self.__apiContext:write(inversePatch) elseif userDecision == "Accept" then - local unappliedPatch = self.__reconciler:applyPatch(catchUpPatch) + local unappliedPatch = self.__reconciler:applyPatch(catchUpPatch, self.__apiContext) if not PatchSet.isEmpty(unappliedPatch) then Log.warn("Could not apply all changes requested by the Rojo server:\n{}", From 24ce366a8153a3b349a1e28dda95d61a017c4f39 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Sat, 16 Sep 2023 20:47:31 -0700 Subject: [PATCH 08/33] Make `fetchInstances` its own module --- plugin/src/Reconciler/fetchInstances.lua | 58 ++++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 plugin/src/Reconciler/fetchInstances.lua diff --git a/plugin/src/Reconciler/fetchInstances.lua b/plugin/src/Reconciler/fetchInstances.lua new file mode 100644 index 000000000..a4f667bbd --- /dev/null +++ b/plugin/src/Reconciler/fetchInstances.lua @@ -0,0 +1,58 @@ +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`. + Log.debug("Loading assets for {:?}", idList) + 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 + for _, entry in map:GetChildren() do + if entry:IsA("ObjectValue") then + local key, value = entry.Name, entry.Value + if value == nil or value.Parent ~= 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.debug("Swapping Instance {} out", oldInstance:GetFullName()) + + local oldParent = oldInstance.Parent + local children = oldInstance:GetChildren() + for _, child in children do + child.Parent = value + end + value.Parent = oldParent + + -- 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 + + end) +end + +return fetchInstances \ No newline at end of file From de3c15b5f18e4604c022fc2eb9f44e2f9038e6f6 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Sat, 16 Sep 2023 21:21:04 -0700 Subject: [PATCH 09/33] Initial implementation of fetchApi in applyPatch --- plugin/src/Reconciler/applyPatch.lua | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index b2aca7dd1..3c449fe70 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -17,6 +17,7 @@ local invariant = require(script.Parent.Parent.invariant) local decodeValue = require(script.Parent.decodeValue) local reify = require(script.Parent.reify) local setProperty = require(script.Parent.setProperty) +local fetchInstances = require(script.Parent.fetchInstances) local function applyPatch(instanceMap, apiContext, patch) local patchTimestamp = DateTime.now():FormatLocalTime("LTS", "en-us") @@ -203,6 +204,21 @@ local function applyPatch(instanceMap, apiContext, patch) end end + -- 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, instanceMap, apiContext) + table.clear(unappliedPatch.updated) + end + ChangeHistoryService:SetWaypoint("Rojo: Patch " .. patchTimestamp) return unappliedPatch From 9c3d84efc091f6217865296a4070361960b37760 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Sat, 16 Sep 2023 21:59:59 -0700 Subject: [PATCH 10/33] Use a folder in the content directory As opposed to a temp dir and hardlinks --- src/serve_session.rs | 22 +--------------------- src/web/api.rs | 45 +++++++++++++++++++------------------------- 2 files changed, 20 insertions(+), 47 deletions(-) diff --git a/src/serve_session.rs b/src/serve_session.rs index a3a8acee7..9dab5446e 100644 --- a/src/serve_session.rs +++ b/src/serve_session.rs @@ -4,14 +4,13 @@ use std::{ io, net::IpAddr, path::{Path, PathBuf}, - sync::{Arc, Mutex, MutexGuard, OnceLock}, + sync::{Arc, Mutex, MutexGuard}, time::Instant, }; use crossbeam_channel::Sender; use memofs::IoResultExt; use memofs::Vfs; -use tempfile::TempDir; use thiserror::Error; use crate::{ @@ -88,10 +87,6 @@ pub struct ServeSession { /// A channel to send mutation requests on. These will be handled by the /// ChangeProcessor and trigger changes in the tree. tree_mutation_sender: Sender, - - /// A temporary directory that is cleaned up at the end of this structure's - /// lifespan. This is not initialized directly to avoid unnecessary work. - temporary_folder: OnceLock>, } impl ServeSession { @@ -165,7 +160,6 @@ impl ServeSession { message_queue, tree_mutation_sender, vfs, - temporary_folder: OnceLock::new(), }) } @@ -225,20 +219,6 @@ impl ServeSession { pub fn root_dir(&self) -> &Path { self.root_project.folder_location() } - - /// Returns a path to a temporary directory if it could be created. - /// Otherwise, returns `None`. - pub fn temp_dir(&self) -> Option<&Path> { - self.temporary_folder - .get_or_init( - || match tempfile::Builder::new().prefix("rojo-").tempdir() { - Ok(dir) => Some(dir), - Err(_) => None, - }, - ) - .as_ref() - .map(|dir| dir.path()) - } } #[derive(Debug, Error)] diff --git a/src/web/api.rs b/src/web/api.rs index 0f68ab8c2..11d877667 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -1,7 +1,7 @@ //! Defines Rojo's HTTP API, all under /api. These endpoints generally return //! JSON. -use std::{collections::HashMap, fs, io::BufWriter, path::PathBuf, str::FromStr, sync::Arc}; +use std::{collections::HashMap, fs, io, io::BufWriter, path::PathBuf, str::FromStr, sync::Arc}; use hyper::{body, Body, Method, Request, Response, StatusCode}; use opener::OpenError; @@ -25,6 +25,8 @@ use crate::{ }, }; +const ROJO_DIR_NAME: &str = ".rojo"; + pub async fn call(serve_session: Arc, request: Request) -> Response { let service = ApiService::new(serve_session); @@ -308,22 +310,27 @@ impl ApiService { } }; - let temp_dir = match self.serve_session.temp_dir() { - Some(dir) => dir, - None => { + let temp_dir = content_dir.join(ROJO_DIR_NAME); + match fs::create_dir(&temp_dir) { + // We want to silently move on if the folder already exists + Err(err) if err.kind() != io::ErrorKind::AlreadyExists => { return json( - ErrorResponse::bad_request("could not create temporary directory"), + ErrorResponse::internal_error(format!( + "Could not create Rojo content directory: {}", + &temp_dir.display() + )), StatusCode::INTERNAL_SERVER_ERROR, - ) + ); } - }; + _ => {} + } let uuid = Uuid::new_v4(); let mut file_name = PathBuf::from(uuid.to_string()); file_name.set_extension("rbxm"); let out_path = temp_dir.join(&file_name); - let studio_path = content_dir.join(&file_name); + let relative_path = PathBuf::from(ROJO_DIR_NAME).join(file_name); let mut writer = BufWriter::new(match fs::File::create(&out_path) { Ok(handle) => handle, @@ -372,24 +379,10 @@ impl ApiService { log::debug!("Wrote model file to {}", out_path.display()); - match fs_err::hard_link(&out_path, &studio_path) { - Ok(_) => { - log::debug!("Created hardlink to {}", &studio_path.display()); - json_ok(FetchResponse { - session_id: self.serve_session.session_id(), - path: file_name.to_string_lossy(), - }) - } - Err(err) => { - log::debug!("Failed to create hardlink to {}", &studio_path.display()); - json( - ErrorResponse::internal_error(format!( - "Could not put file in Roblox content folder: {err}" - )), - StatusCode::SERVICE_UNAVAILABLE, - ) - } - } + json_ok(FetchResponse { + session_id: self.serve_session.session_id(), + path: relative_path.to_string_lossy(), + }) } } From c69ebbe6e95c9a95c05a77b61d7b00ca14cfd103 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Sat, 16 Sep 2023 22:00:17 -0700 Subject: [PATCH 11/33] Move tempfile back to dev dependencies --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index ec972e5c2..4c2109341 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -90,7 +90,6 @@ uuid = { version = "1.0.0", features = ["v4", "serde"] } clap = { version = "3.1.18", features = ["derive"] } profiling = "1.0.6" tracy-client = { version = "0.13.2", optional = true } -tempfile = "3.2.0" [target.'cfg(windows)'.dependencies] winreg = "0.10.1" @@ -112,4 +111,5 @@ insta = { version = "1.8.0", features = ["redactions", "yaml"] } paste = "1.0.5" pretty_assertions = "1.2.1" serde_yaml = "0.8.21" +tempfile = "3.2.0" walkdir = "2.3.2" From 2abac70b77cd07645906fbe856457680062af8de Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Sat, 16 Sep 2023 22:02:57 -0700 Subject: [PATCH 12/33] Handle an invalid request better --- src/web/api.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/web/api.rs b/src/web/api.rs index 11d877667..a185fd81e 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -365,8 +365,10 @@ impl ApiService { .with_name(referent.to_string()), ); } else { - // TODO handle this better - log::error!("bad ref {referent}! ahh!") + return json( + ErrorResponse::bad_request("Invalid ID provided to fetch endpoint"), + StatusCode::BAD_REQUEST, + ); } } if let Err(_) = rbx_binary::to_writer(&mut writer, &sub_tree, &[sub_tree.root_ref()]) { From e057711ae7d5c0362e700963f215ec68f5ab2a03 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Sat, 16 Sep 2023 22:07:16 -0700 Subject: [PATCH 13/33] Rename fetch dir constant --- src/web/api.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/web/api.rs b/src/web/api.rs index a185fd81e..078fff653 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -25,7 +25,7 @@ use crate::{ }, }; -const ROJO_DIR_NAME: &str = ".rojo"; +const FETCH_DIR_NAME: &str = ".rojo"; pub async fn call(serve_session: Arc, request: Request) -> Response { let service = ApiService::new(serve_session); @@ -310,7 +310,7 @@ impl ApiService { } }; - let temp_dir = content_dir.join(ROJO_DIR_NAME); + let temp_dir = content_dir.join(FETCH_DIR_NAME); match fs::create_dir(&temp_dir) { // We want to silently move on if the folder already exists Err(err) if err.kind() != io::ErrorKind::AlreadyExists => { @@ -330,7 +330,7 @@ impl ApiService { file_name.set_extension("rbxm"); let out_path = temp_dir.join(&file_name); - let relative_path = PathBuf::from(ROJO_DIR_NAME).join(file_name); + let relative_path = PathBuf::from(FETCH_DIR_NAME).join(file_name); let mut writer = BufWriter::new(match fs::File::create(&out_path) { Ok(handle) => handle, From 04b34793731a515cd6549616d5445db093fd5009 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Sat, 16 Sep 2023 22:19:47 -0700 Subject: [PATCH 14/33] Actually remove the old copy of fetchInstances from reify --- plugin/src/Reconciler/reify.lua | 52 --------------------------------- 1 file changed, 52 deletions(-) diff --git a/plugin/src/Reconciler/reify.lua b/plugin/src/Reconciler/reify.lua index 37a1583aa..b3c49bb4c 100644 --- a/plugin/src/Reconciler/reify.lua +++ b/plugin/src/Reconciler/reify.lua @@ -159,56 +159,4 @@ function applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch) end end -function fetchInstances(idList, instanceMap, serveSession) - return serveSession.__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`. - Log.debug("Loading assets for {}", idList) - 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 - for _, entry in map:GetChildren() do - if entry:IsA("ObjectValue") then - local key, value = entry.Name, entry.Value - if value == nil or value.Parent ~= 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.debug("Swapping Instance {} out", oldInstance:GetFullName()) - - local oldParent = oldInstance.Parent - local children = oldInstance:GetChildren() - for _, child in children do - child.Parent = value - end - value.Parent = oldParent - - -- 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 - - end) -end - return reify From af7e3729430df47792b286a6b3b951ff73fea8ce Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Mon, 18 Sep 2023 14:19:20 -0700 Subject: [PATCH 15/33] Account for classes with specific parent needs --- src/web/api.rs | 40 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/src/web/api.rs b/src/web/api.rs index 078fff653..02a9270c0 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -356,8 +356,7 @@ impl ApiService { for referent in requested_ids { if inner_tree.get_by_ref(referent).is_some() { log::trace!("Creating clone of {referent} into subtree"); - let new_ref = inner_tree.clone_into_external(referent, &mut sub_tree); - sub_tree.transfer_within(new_ref, reify_ref); + let new_ref = generate_fetch_copy(&inner_tree, &mut sub_tree, reify_ref, referent); sub_tree.insert( map_ref, InstanceBuilder::new("ObjectValue") @@ -416,3 +415,40 @@ fn pick_script_path(instance: InstanceWithMeta<'_>) -> Option { }) .map(|path| path.to_owned()) } + +/// Creates a copy of the Instance pointed to by `referent` from `old_tree`, +/// puts it inside of `new_tree`, and parents it to `parent_ref`. +/// +/// In the event that the Instance is of a class with special parent +/// requirements such as `Attachment`, it will instead make an Instance +/// of the required parent class and place the cloned Instance as a child +/// of that Instance. +/// +/// Regardless, the referent of the clone is returned. +fn generate_fetch_copy( + old_tree: &WeakDom, + new_tree: &mut WeakDom, + parent_ref: Ref, + referent: Ref, +) -> Ref { + let new_ref = old_tree.clone_into_external(referent, new_tree); + let inst = new_tree.get_by_ref(new_ref).unwrap(); + + // Certain classes need to have specific parents otherwise Studio + // doesn't want to load the model. + let real_parent = match inst.class.as_str() { + // These are services, but they're listed here for posterity. + "Terrain" | "StarterPlayerScripts" | "StarterCharacterScripts" => parent_ref, + + "Attachment" => new_tree.insert(parent_ref, InstanceBuilder::new("Part")), + "Bone" => new_tree.insert(parent_ref, InstanceBuilder::new("Part")), + "Animator" => new_tree.insert(parent_ref, InstanceBuilder::new("Humanoid")), + "BaseWrap" => new_tree.insert(parent_ref, InstanceBuilder::new("MeshPart")), + "WrapLayer" => new_tree.insert(parent_ref, InstanceBuilder::new("MeshPart")), + "WrapTarget" => new_tree.insert(parent_ref, InstanceBuilder::new("MeshPart")), + _ => parent_ref, + }; + new_tree.transfer_within(new_ref, real_parent); + + new_ref +} From b8829a0cab2b66ea1fdd8c71d93d162b4f3bc7fa Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Mon, 18 Sep 2023 14:27:56 -0700 Subject: [PATCH 16/33] Add explanatory comment --- src/web/api.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/web/api.rs b/src/web/api.rs index 02a9270c0..efc1bdd97 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -353,6 +353,10 @@ impl ApiService { sub_tree.root_ref(), InstanceBuilder::new("Folder").with_name("ReferentMap"), ); + // Because referents can't be cleanly communicated across a network + // boundary, we have to get creative. So for every Instance we're + // building into a model, an ObjectValue is created's named after the + // old referent and points to the fetched copy of the Instance. for referent in requested_ids { if inner_tree.get_by_ref(referent).is_some() { log::trace!("Creating clone of {referent} into subtree"); From e7788630a49da1841831939959f2d8a5782ec62b Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Mon, 18 Sep 2023 15:27:50 -0700 Subject: [PATCH 17/33] Change how fetchInstances checks invariants --- plugin/src/Reconciler/fetchInstances.lua | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/src/Reconciler/fetchInstances.lua b/plugin/src/Reconciler/fetchInstances.lua index a4f667bbd..e784f1161 100644 --- a/plugin/src/Reconciler/fetchInstances.lua +++ b/plugin/src/Reconciler/fetchInstances.lua @@ -27,7 +27,7 @@ local function fetchInstances(idList, instanceMap, apiContext) for _, entry in map:GetChildren() do if entry:IsA("ObjectValue") then local key, value = entry.Name, entry.Value - if value == nil or value.Parent ~= reified then + 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 From 190d458dddf0ee543888659e8761e4e62856cc7e Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Mon, 18 Sep 2023 15:28:12 -0700 Subject: [PATCH 18/33] Change debug for swapping instances to trace --- plugin/src/Reconciler/fetchInstances.lua | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/src/Reconciler/fetchInstances.lua b/plugin/src/Reconciler/fetchInstances.lua index e784f1161..e630894b0 100644 --- a/plugin/src/Reconciler/fetchInstances.lua +++ b/plugin/src/Reconciler/fetchInstances.lua @@ -35,7 +35,7 @@ local function fetchInstances(idList, instanceMap, apiContext) -- there's no way this results in a data race. local oldInstance: Instance = instanceMap.fromIds[key] instanceMap:insert(key, value) - Log.debug("Swapping Instance {} out", oldInstance:GetFullName()) + Log.trace("Swapping Instance {} out", oldInstance:GetFullName()) local oldParent = oldInstance.Parent local children = oldInstance:GetChildren() From 47dbf1b3f40305de89a29f3da1e00281f11f4990 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Mon, 18 Sep 2023 16:03:44 -0700 Subject: [PATCH 19/33] Use request body instead of URL for fetch endpoint --- plugin/src/ApiContext.lua | 13 +++++++++---- src/web/api.rs | 21 +++++++++++---------- src/web/interface.rs | 7 +++++++ 3 files changed, 27 insertions(+), 14 deletions(-) diff --git a/plugin/src/ApiContext.lua b/plugin/src/ApiContext.lua index fcb99919d..9c0951f35 100644 --- a/plugin/src/ApiContext.lua +++ b/plugin/src/ApiContext.lua @@ -255,15 +255,20 @@ end function ApiContext:fetch(ids: {string}) local url = ("%s/api/fetch/%s"):format(self.__baseUrl, table.concat(ids, ",")) - return Http.get(url) + local requestBody = { + sessionId = self.__sessionId, + idList = ids, + } + + return Http.post(url, Http.jsonEncode(requestBody)) :andThen(rejectFailedRequests) :andThen(Http.Response.json) - :andThen(function(body) - if body.sessionId ~= self.__sessionId then + :andThen(function(responseBody) + if responseBody.sessionId ~= self.__sessionId then return Promise.reject("Server changed ID") end - return body + return responseBody end) end diff --git a/src/web/api.rs b/src/web/api.rs index efc1bdd97..ac9369a0d 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -17,9 +17,9 @@ use crate::{ snapshot::{InstanceWithMeta, PatchSet, PatchUpdate}, web::{ interface::{ - ErrorResponse, FetchResponse, Instance, OpenResponse, ReadResponse, ServerInfoResponse, - SubscribeMessage, SubscribeResponse, WriteRequest, WriteResponse, PROTOCOL_VERSION, - SERVER_VERSION, + ErrorResponse, FetchRequest, FetchResponse, Instance, OpenResponse, ReadResponse, + ServerInfoResponse, SubscribeMessage, SubscribeResponse, WriteRequest, WriteResponse, + PROTOCOL_VERSION, SERVER_VERSION, }, util::{json, json_ok}, }, @@ -45,7 +45,7 @@ pub async fn call(serve_session: Arc, request: Request) -> R (&Method::POST, "/api/write") => service.handle_api_write(request).await, - (&Method::GET, path) if path.starts_with("/api/fetch/") => { + (&Method::POST, path) if path.starts_with("/api/fetch/") => { service.handle_api_fetch_get(request).await } @@ -289,12 +289,13 @@ impl ApiService { } async fn handle_api_fetch_get(&self, request: Request) -> Response { - let argument = &request.uri().path()["/api/fetch/".len()..]; - let requested_ids: Vec = match argument.split(',').map(Ref::from_str).collect() { - Ok(ids) => ids, - Err(_) => { + let body = body::to_bytes(request.into_body()).await.unwrap(); + + let request: FetchRequest = match serde_json::from_slice(&body) { + Ok(request) => request, + Err(err) => { return json( - ErrorResponse::bad_request("Malformed ID list"), + ErrorResponse::bad_request(format!("Malformed request body: {}", err)), StatusCode::BAD_REQUEST, ); } @@ -357,7 +358,7 @@ impl ApiService { // boundary, we have to get creative. So for every Instance we're // building into a model, an ObjectValue is created's named after the // old referent and points to the fetched copy of the Instance. - for referent in requested_ids { + for referent in request.id_list { if inner_tree.get_by_ref(referent).is_some() { log::trace!("Creating clone of {referent} into subtree"); let new_ref = generate_fetch_copy(&inner_tree, &mut sub_tree, reify_ref, referent); diff --git a/src/web/interface.rs b/src/web/interface.rs index 9444be644..893ce1b43 100644 --- a/src/web/interface.rs +++ b/src/web/interface.rs @@ -204,6 +204,13 @@ pub struct OpenResponse { pub session_id: SessionId, } +#[derive(Debug, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FetchRequest { + pub session_id: SessionId, + pub id_list: Vec, +} + /// Response body from GET /api/fetch/{ids} #[derive(Debug, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] From d77e5f273e74b2c188fb72de20567eb1d6a7082d Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Mon, 18 Sep 2023 16:04:19 -0700 Subject: [PATCH 20/33] Remove unhelpful debug log --- plugin/src/Reconciler/fetchInstances.lua | 2 -- 1 file changed, 2 deletions(-) diff --git a/plugin/src/Reconciler/fetchInstances.lua b/plugin/src/Reconciler/fetchInstances.lua index e630894b0..8729d791a 100644 --- a/plugin/src/Reconciler/fetchInstances.lua +++ b/plugin/src/Reconciler/fetchInstances.lua @@ -9,7 +9,6 @@ local function fetchInstances(idList, instanceMap, apiContext) -- 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`. - Log.debug("Loading assets for {:?}", idList) local objects = game:GetObjects("rbxasset://" .. body.path) -- `ReferentMap` is a folder that contains nothing but -- ObjectValues which will be named after entries in `instanceMap` @@ -51,7 +50,6 @@ local function fetchInstances(idList, instanceMap, apiContext) invariant("ReferentMap entry `{}` was a `{}` and not an ObjectValue", entry.Name, entry.ClassName) end end - end) end From e0c43801048ae7a41f15938dd43d4215afae3d41 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Mon, 18 Sep 2023 16:46:56 -0700 Subject: [PATCH 21/33] Reference instances by key instead of name --- plugin/src/Reconciler/fetchInstances.lua | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/src/Reconciler/fetchInstances.lua b/plugin/src/Reconciler/fetchInstances.lua index 8729d791a..bdb75db88 100644 --- a/plugin/src/Reconciler/fetchInstances.lua +++ b/plugin/src/Reconciler/fetchInstances.lua @@ -34,7 +34,7 @@ local function fetchInstances(idList, instanceMap, apiContext) -- 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", oldInstance:GetFullName()) + Log.trace("Swapping Instance {} out", key) local oldParent = oldInstance.Parent local children = oldInstance:GetChildren() From d905679b998df30dc6d27ab7aa7bf34449ba5513 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Mon, 18 Sep 2023 17:22:37 -0700 Subject: [PATCH 22/33] Don't clone subtree when building fetch model --- src/web/api.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/web/api.rs b/src/web/api.rs index ac9369a0d..c7e57c913 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -436,12 +436,19 @@ fn generate_fetch_copy( parent_ref: Ref, referent: Ref, ) -> Ref { - let new_ref = old_tree.clone_into_external(referent, new_tree); - let inst = new_tree.get_by_ref(new_ref).unwrap(); + // We can't use `clone_into_external` here because it also clones the + // subtree + let old_inst = old_tree.get_by_ref(referent).unwrap(); + let new_ref = new_tree.insert( + Ref::none(), + InstanceBuilder::new(&old_inst.class) + .with_name(&old_inst.name) + .with_properties(old_inst.properties.clone()), + ); // Certain classes need to have specific parents otherwise Studio // doesn't want to load the model. - let real_parent = match inst.class.as_str() { + let real_parent = match old_inst.class.as_str() { // These are services, but they're listed here for posterity. "Terrain" | "StarterPlayerScripts" | "StarterCharacterScripts" => parent_ref, From e721e6e0fb38d5bc335774cfe8748c4ea1897432 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Mon, 18 Sep 2023 18:16:39 -0700 Subject: [PATCH 23/33] StyLua --- plugin/src/ApiContext.lua | 2 +- plugin/src/Reconciler/fetchInstances.lua | 87 ++++++++++++------------ 2 files changed, 44 insertions(+), 45 deletions(-) diff --git a/plugin/src/ApiContext.lua b/plugin/src/ApiContext.lua index e99c73e8f..018f9072b 100644 --- a/plugin/src/ApiContext.lua +++ b/plugin/src/ApiContext.lua @@ -239,7 +239,7 @@ function ApiContext:open(id) end) end -function ApiContext:fetch(ids: {string}) +function ApiContext:fetch(ids: { string }) local url = ("%s/api/fetch/%s"):format(self.__baseUrl, table.concat(ids, ",")) local requestBody = { sessionId = self.__sessionId, diff --git a/plugin/src/Reconciler/fetchInstances.lua b/plugin/src/Reconciler/fetchInstances.lua index bdb75db88..e4f90b0c1 100644 --- a/plugin/src/Reconciler/fetchInstances.lua +++ b/plugin/src/Reconciler/fetchInstances.lua @@ -4,53 +4,52 @@ 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 - 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 + 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 + 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) - -- So long and thanks for all the fish :-) - oldInstance:Destroy() + local oldParent = oldInstance.Parent + local children = oldInstance:GetChildren() + for _, child in children do + child.Parent = value end - else - invariant("ReferentMap entry `{}` was a `{}` and not an ObjectValue", entry.Name, entry.ClassName) + value.Parent = oldParent + + -- 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) + end + end) end -return fetchInstances \ No newline at end of file +return fetchInstances From 2c023285c86d1960d344f6ed11c25cfcc87afba8 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Mon, 18 Sep 2023 18:25:24 -0700 Subject: [PATCH 24/33] Preserve selection when replacing instances --- plugin/src/Reconciler/fetchInstances.lua | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/plugin/src/Reconciler/fetchInstances.lua b/plugin/src/Reconciler/fetchInstances.lua index e4f90b0c1..464243335 100644 --- a/plugin/src/Reconciler/fetchInstances.lua +++ b/plugin/src/Reconciler/fetchInstances.lua @@ -1,3 +1,5 @@ +local Selection = game:GetService("Selection") + local Rojo = script:FindFirstAncestor("Rojo") local invariant = require(script.Parent.Parent.invariant) @@ -22,6 +24,14 @@ local function fetchInstances(idList, instanceMap, apiContext) 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 @@ -41,6 +51,12 @@ local function fetchInstances(idList, instanceMap, apiContext) 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() @@ -49,6 +65,8 @@ local function fetchInstances(idList, instanceMap, apiContext) invariant("ReferentMap entry `{}` was a `{}` and not an ObjectValue", entry.Name, entry.ClassName) end end + + Selection:Set(selected) end) end From 6d6e0fe8d95e0ea506082e84be527b014c0f6b80 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Mon, 18 Sep 2023 18:53:26 -0700 Subject: [PATCH 25/33] Cleanup fetch endpoint --- plugin/src/ApiContext.lua | 2 +- src/web/api.rs | 6 ++---- src/web/interface.rs | 3 ++- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/plugin/src/ApiContext.lua b/plugin/src/ApiContext.lua index 018f9072b..19f46ac12 100644 --- a/plugin/src/ApiContext.lua +++ b/plugin/src/ApiContext.lua @@ -240,7 +240,7 @@ function ApiContext:open(id) end function ApiContext:fetch(ids: { string }) - local url = ("%s/api/fetch/%s"):format(self.__baseUrl, table.concat(ids, ",")) + local url = ("%s/api/fetch"):format(self.__baseUrl) local requestBody = { sessionId = self.__sessionId, idList = ids, diff --git a/src/web/api.rs b/src/web/api.rs index c7e57c913..0129bb064 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -45,9 +45,7 @@ pub async fn call(serve_session: Arc, request: Request) -> R (&Method::POST, "/api/write") => service.handle_api_write(request).await, - (&Method::POST, path) if path.starts_with("/api/fetch/") => { - service.handle_api_fetch_get(request).await - } + (&Method::POST, "/api/fetch") => service.handle_api_fetch_post(request).await, (_method, path) => json( ErrorResponse::not_found(format!("Route not found: {}", path)), @@ -288,7 +286,7 @@ impl ApiService { }) } - async fn handle_api_fetch_get(&self, request: Request) -> Response { + async fn handle_api_fetch_post(&self, request: Request) -> Response { let body = body::to_bytes(request.into_body()).await.unwrap(); let request: FetchRequest = match serde_json::from_slice(&body) { diff --git a/src/web/interface.rs b/src/web/interface.rs index 893ce1b43..8c6ad1892 100644 --- a/src/web/interface.rs +++ b/src/web/interface.rs @@ -204,6 +204,7 @@ pub struct OpenResponse { pub session_id: SessionId, } +/// A request POSTed to /api/fetch #[derive(Debug, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct FetchRequest { @@ -211,7 +212,7 @@ pub struct FetchRequest { pub id_list: Vec, } -/// Response body from GET /api/fetch/{ids} +/// Response body from POST /api/fetch #[derive(Debug, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct FetchResponse<'a> { From 6666e11f2faf5563b429014d9af8d33948a4ec92 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Mon, 18 Sep 2023 18:57:26 -0700 Subject: [PATCH 26/33] Remove unused imports in Reify --- plugin/src/Reconciler/reify.lua | 4 ---- 1 file changed, 4 deletions(-) diff --git a/plugin/src/Reconciler/reify.lua b/plugin/src/Reconciler/reify.lua index b3c49bb4c..3c3965734 100644 --- a/plugin/src/Reconciler/reify.lua +++ b/plugin/src/Reconciler/reify.lua @@ -2,15 +2,11 @@ "Reifies" a virtual DOM, constructing a real DOM with the same shape. ]] -local Rojo = script:FindFirstAncestor("Rojo") - 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 Log = require(Rojo.Packages.Log) - local reifyInner, applyDeferredRefs local function reify(instanceMap, virtualInstances, rootId, parentInstance) From d7ace0fcbf2458479a70a89063a2892298d59172 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Mon, 18 Sep 2023 18:58:59 -0700 Subject: [PATCH 27/33] Remove extra argument to `applyPatch` in ServeSession --- plugin/src/ServeSession.lua | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index d5e4fe50d..6d3a204a7 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -279,7 +279,7 @@ function ServeSession:__initialSync(serverInfo) self.__apiContext:write(inversePatch) elseif userDecision == "Accept" then - local unappliedPatch = self.__reconciler:applyPatch(catchUpPatch, self.__apiContext) + local unappliedPatch = self.__reconciler:applyPatch(catchUpPatch) if not PatchSet.isEmpty(unappliedPatch) then Log.warn( From 94cf688a2f79323f00adca49e06c16d9823a035a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bar=C4=B1=C5=9F?= <34089907+Barocena@users.noreply.github.com> Date: Tue, 19 Sep 2023 08:56:39 +0300 Subject: [PATCH 28/33] Create .git-blame-ignore-revs & Add Stylua pr (#787) adds stylua pr to ignore so it doesn't show on git blame history --- .git-blame-ignore-revs | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .git-blame-ignore-revs diff --git a/.git-blame-ignore-revs b/.git-blame-ignore-revs new file mode 100644 index 000000000..dcba947ad --- /dev/null +++ b/.git-blame-ignore-revs @@ -0,0 +1,2 @@ +# stylua formatting +0f8e1625d572a5fe0f7b5c08653ff92cc837d346 From 385340f7561a1108909c3fb8a640ba7189247f07 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Thu, 21 Sep 2023 17:22:43 -0700 Subject: [PATCH 29/33] Add setting to turn functionality off --- plugin/src/App/StatusPages/Settings/init.lua | 13 +++++++++++-- plugin/src/App/init.lua | 2 ++ plugin/src/ServeSession.lua | 2 ++ plugin/src/Settings.lua | 1 + 4 files changed, 16 insertions(+), 2 deletions(-) diff --git a/plugin/src/App/StatusPages/Settings/init.lua b/plugin/src/App/StatusPages/Settings/init.lua index 688be85a3..d91f1e0a7 100644 --- a/plugin/src/App/StatusPages/Settings/init.lua +++ b/plugin/src/App/StatusPages/Settings/init.lua @@ -155,6 +155,15 @@ 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.", + transparency = self.props.transparency, + layoutOrder = 6, + }), + OpenScriptsExternally = e(Setting, { id = "openScriptsExternally", name = "Open Scripts Externally", @@ -162,7 +171,7 @@ function SettingsPage:render() locked = self.props.syncActive, experimental = true, transparency = self.props.transparency, - layoutOrder = 6, + layoutOrder = 7, }), TwoWaySync = e(Setting, { @@ -172,7 +181,7 @@ function SettingsPage:render() locked = self.props.syncActive, experimental = true, transparency = self.props.transparency, - layoutOrder = 7, + layoutOrder = 8, }), LogLevel = e(Setting, { diff --git a/plugin/src/App/init.lua b/plugin/src/App/init.lua index cfebcad8c..1d96568ae 100644 --- a/plugin/src/App/init.lua +++ b/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) diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index 6d3a204a7..5bb825f14 100644 --- a/plugin/src/ServeSession.lua +++ b/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) @@ -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, diff --git a/plugin/src/Settings.lua b/plugin/src/Settings.lua index 194dd4c7a..680a2494a 100644 --- a/plugin/src/Settings.lua +++ b/plugin/src/Settings.lua @@ -13,6 +13,7 @@ local defaultSettings = { openScriptsExternally = false, twoWaySync = false, showNotifications = true, + fetchOnPatchFail = true, syncReminder = true, confirmationBehavior = "Initial", largeChangesConfirmationThreshold = 5, From 81a5c0fff3011f39fc522c02ea361f9d00678c00 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Thu, 21 Sep 2023 17:27:01 -0700 Subject: [PATCH 30/33] Lock fetchOnPatchFail setting while sync is active --- plugin/src/App/StatusPages/Settings/init.lua | 1 + 1 file changed, 1 insertion(+) diff --git a/plugin/src/App/StatusPages/Settings/init.lua b/plugin/src/App/StatusPages/Settings/init.lua index d91f1e0a7..6c4a4b97f 100644 --- a/plugin/src/App/StatusPages/Settings/init.lua +++ b/plugin/src/App/StatusPages/Settings/init.lua @@ -160,6 +160,7 @@ function SettingsPage:render() 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, }), From 424b1d0d8b1a7b6838f1a87019de69ae5501159f Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Thu, 21 Sep 2023 17:40:21 -0700 Subject: [PATCH 31/33] Move ChangeHistoryService waypoint creation to Reconciler --- plugin/src/Reconciler/applyPatch.lua | 6 ------ plugin/src/Reconciler/init.lua | 5 +++++ 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index 3c449fe70..2e81f6d57 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/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) @@ -20,8 +18,6 @@ local setProperty = require(script.Parent.setProperty) local fetchInstances = require(script.Parent.fetchInstances) local function applyPatch(instanceMap, apiContext, 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() @@ -219,8 +215,6 @@ local function applyPatch(instanceMap, apiContext, patch) table.clear(unappliedPatch.updated) end - ChangeHistoryService:SetWaypoint("Rojo: Patch " .. patchTimestamp) - return unappliedPatch end diff --git a/plugin/src/Reconciler/init.lua b/plugin/src/Reconciler/init.lua index cf491aed6..11c509ed6 100644 --- a/plugin/src/Reconciler/init.lua +++ b/plugin/src/Reconciler/init.lua @@ -2,6 +2,7 @@ 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) @@ -66,8 +67,12 @@ function Reconciler:applyPatch(patch) end end + local patchTimestamp = DateTime.now():FormatLocalTime("LTS", "en-us") + local unappliedPatch = applyPatch(self.__instanceMap, self.__apiContext, patch) + ChangeHistoryService:SetWaypoint("Rojo: Patch " .. patchTimestamp) + for _, callback in self.__postcommitCallbacks do local success, err = pcall(callback, patch, self.__instanceMap, unappliedPatch) if not success then From a5321bea5d323d43a85c817aeb678e04dac0fcf1 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Thu, 21 Sep 2023 17:48:00 -0700 Subject: [PATCH 32/33] Move fetchinstances call to reconciler --- plugin/src/Reconciler/applyPatch.lua | 18 +----------------- plugin/src/Reconciler/init.lua | 20 +++++++++++++++++++- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index 2e81f6d57..a625ebe93 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -15,9 +15,8 @@ local invariant = require(script.Parent.Parent.invariant) local decodeValue = require(script.Parent.decodeValue) local reify = require(script.Parent.reify) local setProperty = require(script.Parent.setProperty) -local fetchInstances = require(script.Parent.fetchInstances) -local function applyPatch(instanceMap, apiContext, patch) +local function applyPatch(instanceMap, patch) -- Tracks any portions of the patch that could not be applied to the DOM. local unappliedPatch = PatchSet.newEmpty() @@ -200,21 +199,6 @@ local function applyPatch(instanceMap, apiContext, patch) end end - -- 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, instanceMap, apiContext) - table.clear(unappliedPatch.updated) - end - return unappliedPatch end diff --git a/plugin/src/Reconciler/init.lua b/plugin/src/Reconciler/init.lua index 11c509ed6..df315491c 100644 --- a/plugin/src/Reconciler/init.lua +++ b/plugin/src/Reconciler/init.lua @@ -7,6 +7,9 @@ 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) @@ -69,7 +72,22 @@ function Reconciler:applyPatch(patch) local patchTimestamp = DateTime.now():FormatLocalTime("LTS", "en-us") - local unappliedPatch = applyPatch(self.__instanceMap, self.__apiContext, patch) + local unappliedPatch = applyPatch(self.__instanceMap, patch) + + -- 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 ChangeHistoryService:SetWaypoint("Rojo: Patch " .. patchTimestamp) From d21ffe1f925e4742ac0b8e680f26906af0358a88 Mon Sep 17 00:00:00 2001 From: Dekkonot Date: Thu, 21 Sep 2023 17:53:03 -0700 Subject: [PATCH 33/33] Respect setting for fetch usage in reconciler --- plugin/src/Reconciler/init.lua | 29 ++++++++++++++++------------- plugin/src/ServeSession.lua | 2 +- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/plugin/src/Reconciler/init.lua b/plugin/src/Reconciler/init.lua index df315491c..3eae4759c 100644 --- a/plugin/src/Reconciler/init.lua +++ b/plugin/src/Reconciler/init.lua @@ -17,12 +17,13 @@ local diff = require(script.diff) local Reconciler = {} Reconciler.__index = Reconciler -function Reconciler.new(instanceMap, apiContext) +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 = {}, } @@ -74,19 +75,21 @@ function Reconciler:applyPatch(patch) local unappliedPatch = applyPatch(self.__instanceMap, patch) - -- 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 + 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 - -- 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 ChangeHistoryService:SetWaypoint("Rojo: Patch " .. patchTimestamp) diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index 5bb825f14..3bd82079c 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -74,7 +74,7 @@ function ServeSession.new(options) local instanceMap = InstanceMap.new(onInstanceChanged) local changeBatcher = ChangeBatcher.new(instanceMap, onChangesFlushed) - local reconciler = Reconciler.new(instanceMap, options.apiContext) + local reconciler = Reconciler.new(instanceMap, options.apiContext, options.fetchOnPatchFail) local connections = {}