From 782843a5369d4be70e81cee4333c72686f53dfb2 Mon Sep 17 00:00:00 2001 From: Nolan-O Date: Tue, 9 Jul 2024 16:44:21 -0400 Subject: [PATCH 1/5] Add support for `$paths` field in folders Allows a folder's contents to be the sum of multiple folders' contents Ideal for composing forms of a project depending on rojo's project files e.g. multi-place games, libraries with tiered features for releases, etc --- .../multiple_paths/default.project.json | 9 ++ .../serve-tests/multiple_paths/src1/eg1.luau | 0 .../serve-tests/multiple_paths/src2/eg2.luau | 0 src/project.rs | 3 + src/snapshot_middleware/project.rs | 83 ++++++++++++++++--- 5 files changed, 85 insertions(+), 10 deletions(-) create mode 100644 rojo-test/serve-tests/multiple_paths/default.project.json create mode 100644 rojo-test/serve-tests/multiple_paths/src1/eg1.luau create mode 100644 rojo-test/serve-tests/multiple_paths/src2/eg2.luau diff --git a/rojo-test/serve-tests/multiple_paths/default.project.json b/rojo-test/serve-tests/multiple_paths/default.project.json new file mode 100644 index 000000000..a1e72927d --- /dev/null +++ b/rojo-test/serve-tests/multiple_paths/default.project.json @@ -0,0 +1,9 @@ +{ + "name": "multiple_paths", + "tree": { + "$paths": [ + "src1", + "src2" + ] + } +} \ No newline at end of file diff --git a/rojo-test/serve-tests/multiple_paths/src1/eg1.luau b/rojo-test/serve-tests/multiple_paths/src1/eg1.luau new file mode 100644 index 000000000..e69de29bb diff --git a/rojo-test/serve-tests/multiple_paths/src2/eg2.luau b/rojo-test/serve-tests/multiple_paths/src2/eg2.luau new file mode 100644 index 000000000..e69de29bb diff --git a/src/project.rs b/src/project.rs index 6e503dd63..3e2cf794c 100644 --- a/src/project.rs +++ b/src/project.rs @@ -278,6 +278,9 @@ pub struct ProjectNode { /// spreadsheets (`.csv`). #[serde(rename = "$path", skip_serializing_if = "Option::is_none")] pub path: Option, + + #[serde(rename = "$paths", skip_serializing_if = "Option::is_none")] + pub paths: Option>, } impl ProjectNode { diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index 53e65251b..72fbcbdc8 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -128,6 +128,54 @@ pub fn snapshot_project_node( // on. metadata = snapshot.metadata; } + } else if let Some(paths) = &node.paths { + let mut relevant_paths = vec![]; + class_name_from_path = Some(Cow::Borrowed("Folder")); + + for path_node in paths { + let path = path_node.path(); + + // As above, assume relative paths are relative to project_folder + let full_path = if path.is_relative() { + Cow::Owned(project_folder.join(path)) + } else { + Cow::Borrowed(path) + }; + + if let Some(snapshot) = snapshot_from_vfs(context, vfs, &full_path)? { + // Since class_name_from_path is a single value, we can't check + // it in the inference-checking block below for multiple paths. + // Thus we bail here if it is ever something other than folder + if snapshot.class_name != "Folder" { + bail!( + "Instance \"{}\" has $paths values which are not of type Folder.\n\ + All inferred class names for paths in $paths must be of type Folder.\n\ + Found: {}\n\ + At: {}\n\ + Project path: {}", + instance_name, + snapshot.class_name, + full_path.display(), + project_path.display() + ); + } + + // Combine all relevant paths of our children into us + // Otherwise, parts of our children can be destroyed but + // not recreated or modified + for relevant_path in snapshot.metadata.relevant_paths.iter() { + relevant_paths.push(relevant_path.to_path_buf()); + } + + // As above, merge the snapshot's children into ours + children.reserve(snapshot.children.len()); + for child in snapshot.children.into_iter() { + children.push(child); + } + } + } + + metadata.relevant_paths = relevant_paths; } let class_name_from_inference = infer_class_name(&name, parent_class); @@ -137,20 +185,21 @@ pub fn snapshot_project_node( class_name_from_path, class_name_from_inference, &node.path, + &node.paths, ) { // These are the easy, happy paths! - (Some(project), None, None, _) => project, - (None, Some(path), None, _) => path, - (None, None, Some(inference), _) => inference, + (Some(project), None, None, _, _) => project, + (None, Some(path), None, _, _) => path, + (None, None, Some(inference), _, _) => inference, // If the user specifies a class name, but there's an inferred class // name, we prefer the name listed explicitly by the user. - (Some(project), None, Some(_), _) => project, + (Some(project), None, Some(_), _, _) => project, // If the user has a $path pointing to a folder and we're able to infer // a class name, let's use the inferred name. If the path we're pointing // to isn't a folder, though, that's a user error. - (None, Some(path), Some(inference), _) => { + (None, Some(path), Some(inference), _, _) => { if path == "Folder" { inference } else { @@ -158,7 +207,7 @@ pub fn snapshot_project_node( } } - (Some(project), Some(path), _, _) => { + (Some(project), Some(path), _, _, _) => { if path == "Folder" { project } else { @@ -177,11 +226,15 @@ pub fn snapshot_project_node( } } - (None, None, None, Some(PathNode::Optional(_))) => { + (None, None, None, Some(PathNode::Optional(_)), None) => { return Ok(None); } - (_, None, _, Some(PathNode::Required(path))) => { + (None, None, None, None, Some(_)) => { + return Ok(None); + } + + (_, None, _, Some(PathNode::Required(path)), None) => { anyhow::bail!( "Rojo project referred to a file using $path that could not be turned into a Roblox Instance by Rojo.\n\ Check that the file exists and is a file type known by Rojo.\n\ @@ -193,12 +246,13 @@ pub fn snapshot_project_node( ); } - (None, None, None, None) => { + (None, None, None, None, None) => { bail!( "Instance \"{}\" is missing some required information.\n\ One of the following must be true:\n\ - $className must be set to the name of a Roblox class\n\ - $path must be set to a path of an instance\n\ + - $paths must be set to a list of folders\n\ - The instance must be a known service, like ReplicatedStorage\n\ \n\ Project path: {}", @@ -206,6 +260,15 @@ pub fn snapshot_project_node( project_path.display(), ); } + (None, None, None, Some(_), Some(_)) => { + bail!( + "Instance \"{}\" has both $path and $paths set.\n\ + rojo has different rules for behavior of these fields, so they cannot be combined.\n\ + Project path: {}\n", + instance_name, + project_path.display(), + ) + } }; for (child_name, child_project_node) in &node.children { @@ -274,7 +337,7 @@ pub fn snapshot_project_node( // file), set it to true. if let Some(ignore) = node.ignore_unknown_instances { metadata.ignore_unknown_instances = ignore; - } else if node.path.is_none() { + } else if node.path.is_none() || node.paths.is_some() { // TODO: Introduce a strict mode where $ignoreUnknownInstances is never // set implicitly. metadata.ignore_unknown_instances = true; From c12657339b8c34749e9e63ee68de40acdc586b3c Mon Sep 17 00:00:00 2001 From: Nolan-O Date: Tue, 9 Jul 2024 19:27:31 -0400 Subject: [PATCH 2/5] Add serve-test for `$paths` --- ...end__tests__serve__multiple_paths_all.snap | 41 +++++++++++ ...nd__tests__serve__multiple_paths_info.snap | 13 ++++ ...ts__serve__multiple_paths_rename1_all.snap | 41 +++++++++++ ...rve__multiple_paths_rename1_subscribe.snap | 26 +++++++ ...ts__serve__multiple_paths_rename2_all.snap | 41 +++++++++++ ...rve__multiple_paths_rename2_subscribe.snap | 40 +++++++++++ tests/tests/serve.rs | 69 +++++++++++++++++++ 7 files changed, 271 insertions(+) create mode 100644 rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_all.snap create mode 100644 rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_info.snap create mode 100644 rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_rename1_all.snap create mode 100644 rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_rename1_subscribe.snap create mode 100644 rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_rename2_all.snap create mode 100644 rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_rename2_subscribe.snap diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_all.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_all.snap new file mode 100644 index 000000000..b665b94a6 --- /dev/null +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_all.snap @@ -0,0 +1,41 @@ +--- +source: tests/tests/serve.rs +expression: "read_response.intern_and_redact(&mut redactions, root_id)" + +--- +instances: + id-2: + Children: + - id-3 + - id-4 + ClassName: Folder + Id: id-2 + Metadata: + ignoreUnknownInstances: true + Name: multiple_paths + Parent: "00000000000000000000000000000000" + Properties: {} + id-3: + Children: [] + ClassName: ModuleScript + Id: id-3 + Metadata: + ignoreUnknownInstances: false + Name: eg1 + Parent: id-2 + Properties: + Source: + String: "" + id-4: + Children: [] + ClassName: ModuleScript + Id: id-4 + Metadata: + ignoreUnknownInstances: false + Name: eg2 + Parent: id-2 + Properties: + Source: + String: "" +messageCursor: 0 +sessionId: id-1 diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_info.snap new file mode 100644 index 000000000..32bc59126 --- /dev/null +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_info.snap @@ -0,0 +1,13 @@ +--- +source: tests/tests/serve.rs +expression: redactions.redacted_yaml(info) + +--- +expectedPlaceIds: ~ +gameId: ~ +placeId: ~ +projectName: multiple_paths +protocolVersion: 4 +rootInstanceId: id-2 +serverVersion: "[server-version]" +sessionId: id-1 diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_rename1_all.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_rename1_all.snap new file mode 100644 index 000000000..d57461ff8 --- /dev/null +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_rename1_all.snap @@ -0,0 +1,41 @@ +--- +source: tests/tests/serve.rs +expression: "read_response.intern_and_redact(&mut redactions, root_id)" + +--- +instances: + id-2: + Children: + - id-4 + - id-5 + ClassName: Folder + Id: id-2 + Metadata: + ignoreUnknownInstances: true + Name: multiple_paths + Parent: "00000000000000000000000000000000" + Properties: {} + id-4: + Children: [] + ClassName: ModuleScript + Id: id-4 + Metadata: + ignoreUnknownInstances: false + Name: eg2 + Parent: id-2 + Properties: + Source: + String: "" + id-5: + Children: [] + ClassName: ModuleScript + Id: id-5 + Metadata: + ignoreUnknownInstances: false + Name: eg11 + Parent: id-2 + Properties: + Source: + String: "" +messageCursor: 2 +sessionId: id-1 diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_rename1_subscribe.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_rename1_subscribe.snap new file mode 100644 index 000000000..5328cc878 --- /dev/null +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_rename1_subscribe.snap @@ -0,0 +1,26 @@ +--- +source: tests/tests/serve.rs +expression: "subscribe_response.intern_and_redact(&mut redactions, ())" + +--- +messageCursor: 2 +messages: + - added: {} + removed: + - id-3 + updated: [] + - added: + id-5: + Children: [] + ClassName: ModuleScript + Id: id-5 + Metadata: + ignoreUnknownInstances: false + Name: eg11 + Parent: id-2 + Properties: + Source: + String: "" + removed: [] + updated: [] +sessionId: id-1 diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_rename2_all.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_rename2_all.snap new file mode 100644 index 000000000..b7210f6c6 --- /dev/null +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_rename2_all.snap @@ -0,0 +1,41 @@ +--- +source: tests/tests/serve.rs +expression: "read_response.intern_and_redact(&mut redactions, root_id)" + +--- +instances: + id-2: + Children: + - id-5 + - id-6 + ClassName: Folder + Id: id-2 + Metadata: + ignoreUnknownInstances: true + Name: multiple_paths + Parent: "00000000000000000000000000000000" + Properties: {} + id-5: + Children: [] + ClassName: ModuleScript + Id: id-5 + Metadata: + ignoreUnknownInstances: false + Name: eg11 + Parent: id-2 + Properties: + Source: + String: "" + id-6: + Children: [] + ClassName: ModuleScript + Id: id-6 + Metadata: + ignoreUnknownInstances: false + Name: eg22 + Parent: id-2 + Properties: + Source: + String: "" +messageCursor: 4 +sessionId: id-1 diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_rename2_subscribe.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_rename2_subscribe.snap new file mode 100644 index 000000000..5f7b72131 --- /dev/null +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__multiple_paths_rename2_subscribe.snap @@ -0,0 +1,40 @@ +--- +source: tests/tests/serve.rs +expression: "subscribe_response.intern_and_redact(&mut redactions, ())" + +--- +messageCursor: 4 +messages: + - added: + id-5: + Children: [] + ClassName: ModuleScript + Id: id-5 + Metadata: + ignoreUnknownInstances: false + Name: eg11 + Parent: id-2 + Properties: + Source: + String: "" + removed: [] + updated: [] + - added: {} + removed: + - id-4 + updated: [] + - added: + id-6: + Children: [] + ClassName: ModuleScript + Id: id-6 + Metadata: + ignoreUnknownInstances: false + Name: eg22 + Parent: id-2 + Properties: + Source: + String: "" + removed: [] + updated: [] +sessionId: id-1 diff --git a/tests/tests/serve.rs b/tests/tests/serve.rs index 2e8d61b8e..efce5972c 100644 --- a/tests/tests/serve.rs +++ b/tests/tests/serve.rs @@ -445,3 +445,72 @@ fn ref_properties_remove() { ); }); } + +#[test] +fn multiple_paths_serve() { + run_serve_test("multiple_paths", |session, mut redactions| { + let info = session.get_api_rojo().unwrap(); + let root_id = info.root_instance_id; + + assert_yaml_snapshot!("multiple_paths_info", redactions.redacted_yaml(info)); + + let first_read_response = session.get_api_read(root_id).unwrap(); + assert_yaml_snapshot!( + "multiple_paths_all", + first_read_response.intern_and_redact(&mut redactions, root_id) + ); + }) +} + +#[test] +fn multiple_paths_updates() { + run_serve_test("multiple_paths", |session, mut redactions| { + let info = session.get_api_rojo().unwrap(); + let root_id = info.root_instance_id; + + assert_yaml_snapshot!("multiple_paths_info", redactions.redacted_yaml(info)); + + let first_read_response = session.get_api_read(root_id).unwrap(); + assert_yaml_snapshot!( + "multiple_paths_all", + first_read_response.intern_and_redact(&mut redactions, root_id) + ); + + // Rename the files in both paths + fs::rename( + session.path().join("src1/eg1.luau"), + session.path().join("src2/eg11.luau"), + ) + .unwrap(); + + let subscribe_response = session.get_api_subscribe(0).unwrap(); + assert_yaml_snapshot!( + "multiple_paths_rename1_subscribe", + subscribe_response.intern_and_redact(&mut redactions, ()) + ); + + let read_response = session.get_api_read(root_id).unwrap(); + assert_yaml_snapshot!( + "multiple_paths_rename1_all", + read_response.intern_and_redact(&mut redactions, root_id) + ); + + fs::rename( + session.path().join("src2/eg2.luau"), + session.path().join("src2/eg22.luau"), + ) + .unwrap(); + + let subscribe_response = session.get_api_subscribe(1).unwrap(); + assert_yaml_snapshot!( + "multiple_paths_rename2_subscribe", + subscribe_response.intern_and_redact(&mut redactions, ()) + ); + + let read_response = session.get_api_read(root_id).unwrap(); + assert_yaml_snapshot!( + "multiple_paths_rename2_all", + read_response.intern_and_redact(&mut redactions, root_id) + ); + }) +} From 07d9f38a381e69abd5a7812e9e34d02c33d0c9dc Mon Sep 17 00:00:00 2001 From: Nolan-O Date: Tue, 9 Jul 2024 19:50:42 -0400 Subject: [PATCH 3/5] bail if no inferred type for `$paths`-containing node --- src/snapshot_middleware/project.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index 72fbcbdc8..fcaa3bbc3 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -231,7 +231,11 @@ pub fn snapshot_project_node( } (None, None, None, None, Some(_)) => { - return Ok(None); + bail!( + "Instance \"{}\" has $paths set but no type was inferred.\n\ + This is a bug. Please file an issue!", + instance_name + ) } (_, None, _, Some(PathNode::Required(path)), None) => { From cad3d3e02e2f7152abed02f15660cf33d162847d Mon Sep 17 00:00:00 2001 From: Nolan-O Date: Tue, 9 Jul 2024 20:05:40 -0400 Subject: [PATCH 4/5] bail if specified $classname isn't Folder for $paths nodes --- src/snapshot_middleware/project.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index fcaa3bbc3..567a21069 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -207,6 +207,20 @@ pub fn snapshot_project_node( } } + (Some(project), _, _, None, Some(_)) => { + if project == "Folder" { + project + } else { + bail!( + "Instance \"{}\" has has $classname \"{}\", but $paths can only be used on Folder classes.\n\ + Project path: {}", + instance_name, + project, + project_path.display() + ) + } + } + (Some(project), Some(path), _, _, _) => { if path == "Folder" { project @@ -268,7 +282,7 @@ pub fn snapshot_project_node( bail!( "Instance \"{}\" has both $path and $paths set.\n\ rojo has different rules for behavior of these fields, so they cannot be combined.\n\ - Project path: {}\n", + Project path: {}", instance_name, project_path.display(), ) From af39d87bea19ba51612a328292eb24374a530910 Mon Sep 17 00:00:00 2001 From: Nolan-O Date: Tue, 9 Jul 2024 20:44:32 -0400 Subject: [PATCH 5/5] Update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad9b44bbb..75892bb73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ * Added experimental setting for Auto Connect in playtests ([#840]) * Improved settings UI ([#886]) * `Open Scripts Externally` option can now be changed while syncing ([#911]) +* Folders can specify `$paths` rather than `$path` to combine multiple folders on the filesystem ([#936]) * Projects may now specify rules for syncing files as if they had a different file extension. ([#813]) This is specified via a new field on project files, `syncRules`: @@ -87,6 +88,7 @@ [#903]: https://github.com/rojo-rbx/rojo/pull/903 [#911]: https://github.com/rojo-rbx/rojo/pull/911 [#915]: https://github.com/rojo-rbx/rojo/pull/915 +[#936]: https://github.com/rojo-rbx/rojo/pull/936 ## [7.4.1] - February 20, 2024 * Made the `name` field optional on project files ([#870])