From 7e60406373faf82887353164543540d054d84d92 Mon Sep 17 00:00:00 2001 From: boyned//Kampfkarren <3190756+Kampfkarren@users.noreply.github.com> Date: Mon, 5 Oct 2026 18:02:34 -0700 Subject: [PATCH] Consistent ordering of CSV fields (#1333) Paired with https://github.com/rojo-rbx/rbx-dom/pull/668 This fixes a bug where the first sync of a place with LocalizationTables would always fail diff, since Roblox computes it at runtime --- CHANGELOG.md | 4 + ...end_to_end__tests__build__csv_bug_145.snap | 2 +- ...end_to_end__tests__build__csv_bug_147.snap | 2 +- ...d_to_end__tests__build__csv_in_folder.snap | 2 +- src/snapshot_middleware/csv.rs | 81 ++++++++++++++++--- ...t_middleware__csv__test__csv_from_vfs.snap | 2 +- ...pshot_middleware__csv__test__csv_init.snap | 2 +- ...leware__csv__test__csv_init_with_meta.snap | 2 +- ..._middleware__csv__test__csv_with_meta.snap | 2 +- 9 files changed, 80 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d7782a5c..3943ae5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,10 @@ Making a new release? Simply add the new header with the version and date undern ## Unreleased +* LocalizationTables no longer report as changed when nothing changes. [(#1333)] + +[#1333]: https://github.com/rojo-rbx/rojo/pull/1333 + ## [7.7.1] (October 1st, 2026) * Fixed `$path` values that point outside the project folder failing to match `syncRule`s on Windows, which broke `rojo sourcemap` with a "could not be turned into a Roblox Instance" error. ([#1290]) diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_bug_145.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_bug_145.snap index ab7470b3..96cdc002 100644 --- a/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_bug_145.snap +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_bug_145.snap @@ -10,7 +10,7 @@ expression: contents normal - [{"key":"Count","example":"A number demonstrating issue 145","source":"3","values":{"es":"7"}}] + [{"example":"A number demonstrating issue 145","key":"Count","source":"3","values":{"es":"7"}}] diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_bug_147.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_bug_147.snap index d16de20f..8a668402 100644 --- a/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_bug_147.snap +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_bug_147.snap @@ -10,7 +10,7 @@ expression: contents normal - [{"key":"Language.Name","source":"English","values":{}},{"key":"Language.Region","source":"United States","values":{}},{"key":"Label.Thickness","source":"Thickness","values":{}},{"key":"Label.Opacity","source":"Opacity","values":{}},{"key":"Toolbar.Undo","source":"Undo","values":{}},{"key":"Toolbar.Redo","source":"Redo","values":{}},{"key":"Toolbar.Camera","source":"Top-down camera","values":{}},{"key":"Toolbar.Saves","source":"Saved drawings","values":{}},{"key":"Toolbar.Preferences","source":"Settings","values":{}},{"key":"Toolbar.Mode.Vector","source":"Vector mode","values":{}},{"key":"Toolbar.Mode.Pixel","source":"Pixel mode","values":{}}] + [{"key":"Label.Opacity","source":"Opacity","values":{}},{"key":"Label.Thickness","source":"Thickness","values":{}},{"key":"Language.Name","source":"English","values":{}},{"key":"Language.Region","source":"United States","values":{}},{"key":"Toolbar.Camera","source":"Top-down camera","values":{}},{"key":"Toolbar.Mode.Pixel","source":"Pixel mode","values":{}},{"key":"Toolbar.Mode.Vector","source":"Vector mode","values":{}},{"key":"Toolbar.Preferences","source":"Settings","values":{}},{"key":"Toolbar.Redo","source":"Redo","values":{}},{"key":"Toolbar.Saves","source":"Saved drawings","values":{}},{"key":"Toolbar.Undo","source":"Undo","values":{}}] diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_in_folder.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_in_folder.snap index d60fca02..40bc7ce5 100644 --- a/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_in_folder.snap +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_in_folder.snap @@ -10,7 +10,7 @@ expression: contents normal - [{"key":"Ack","example":"An exclamation of despair","source":"Ack!","values":{"es":"¡Ay!"}}] + [{"example":"An exclamation of despair","key":"Ack","source":"Ack!","values":{"es":"¡Ay!"}}] diff --git a/src/snapshot_middleware/csv.rs b/src/snapshot_middleware/csv.rs index b4a86efc..8cea9b81 100644 --- a/src/snapshot_middleware/csv.rs +++ b/src/snapshot_middleware/csv.rs @@ -162,13 +162,11 @@ pub fn syncback_csv_init<'sync>( /// /// We manually deserialize into this table from CSV, but let serde_json handle /// serialization. -#[derive(Debug, Default, Serialize, Deserialize)] +#[derive(Debug, Default, Deserialize)] #[serde(rename_all = "camelCase")] struct LocalizationEntry<'a> { - #[serde(skip_serializing_if = "Option::is_none")] key: Option>, - #[serde(skip_serializing_if = "Option::is_none")] context: Option>, // Roblox writes `examples` for LocalizationTable's Content property, which @@ -176,16 +174,77 @@ struct LocalizationEntry<'a> { // This is reported here: https://devforum.roblox.com/t/2908720. // // To support their mistake, we support an alias named `examples`. - #[serde(skip_serializing_if = "Option::is_none", alias = "examples")] + #[serde(alias = "examples")] example: Option>, - #[serde(skip_serializing_if = "Option::is_none")] source: Option>, // We use a BTreeMap here to get deterministic output order. values: BTreeMap, Cow<'a, str>>, } +// Guarantee a specific order of both fields and entries so that diff always match up +impl<'a> Serialize for LocalizationEntry<'a> { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + let mut btree = BTreeMap::new(); + + if let Some(key) = &self.key { + btree.insert( + "key", + serde_json::to_value(key).map_err(serde::ser::Error::custom)?, + ); + } + + if let Some(context) = &self.context { + btree.insert( + "context", + serde_json::to_value(context).map_err(serde::ser::Error::custom)?, + ); + } + + if let Some(example) = &self.example { + btree.insert( + "example", + serde_json::to_value(example).map_err(serde::ser::Error::custom)?, + ); + } + + if let Some(source) = &self.source { + btree.insert( + "source", + serde_json::to_value(source).map_err(serde::ser::Error::custom)?, + ); + } + + btree.insert( + "values", + serde_json::to_value(&self.values).map_err(serde::ser::Error::custom)?, + ); + + btree.serialize(serializer) + } +} + +fn sort_localization_entries(mut entries: Vec) -> Vec { + entries.sort_by(|a, b| { + a.key + .as_deref() + .unwrap_or_default() + .cmp(&b.key.as_deref().unwrap_or_default()) + .then_with(|| { + a.source + .as_deref() + .unwrap_or_default() + .cmp(&b.source.as_deref().unwrap_or_default()) + }) + }); + + entries +} + /// Normally, we'd be able to let the csv crate construct our struct for us. /// /// However, because of a limitation with Serde's 'flatten' feature, it's not @@ -236,8 +295,8 @@ fn convert_localization_csv(contents: &[u8]) -> anyhow::Result { entries.push(entry); } - let encoded = - serde_json::to_string(&entries).context("Could not encode JSON for localization table")?; + let encoded = serde_json::to_string(&sort_localization_entries(entries)) + .context("Could not encode JSON for localization table")?; Ok(encoded) } @@ -249,11 +308,9 @@ fn localization_to_csv(csv_contents: &str) -> anyhow::Result> { let mut out = Vec::new(); let mut writer = csv::Writer::from_writer(&mut out); - let mut csv: Vec = - serde_json::from_str(csv_contents).context("cannot decode JSON from localization table")?; - - // TODO sort this better - csv.sort_by(|a, b| a.source.partial_cmp(&b.source).unwrap()); + let csv: Vec = sort_localization_entries( + serde_json::from_str(csv_contents).context("cannot decode JSON from localization table")?, + ); let mut headers = vec!["Key", "Source", "Context", "Example"]; // We want both order and a lack of duplicates, so we use a BTreeSet. diff --git a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_from_vfs.snap b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_from_vfs.snap index 6356256d..aa4c18c8 100644 --- a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_from_vfs.snap +++ b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_from_vfs.snap @@ -20,5 +20,5 @@ name: foo class_name: LocalizationTable properties: Contents: - String: "[{\"key\":\"Ack\",\"example\":\"An exclamation of despair\",\"source\":\"Ack!\",\"values\":{\"es\":\"¡Ay!\"}}]" + String: "[{\"example\":\"An exclamation of despair\",\"key\":\"Ack\",\"source\":\"Ack!\",\"values\":{\"es\":\"¡Ay!\"}}]" children: [] diff --git a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init.snap b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init.snap index 813a2933..385666a8 100644 --- a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init.snap +++ b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init.snap @@ -29,5 +29,5 @@ name: root class_name: LocalizationTable properties: Contents: - String: "[{\"key\":\"Ack\",\"example\":\"An exclamation of despair\",\"source\":\"Ack!\",\"values\":{\"es\":\"¡Ay!\"}}]" + String: "[{\"example\":\"An exclamation of despair\",\"key\":\"Ack\",\"source\":\"Ack!\",\"values\":{\"es\":\"¡Ay!\"}}]" children: [] diff --git a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init_with_meta.snap b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init_with_meta.snap index 01ce0a0f..81fe3351 100644 --- a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init_with_meta.snap +++ b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init_with_meta.snap @@ -29,5 +29,5 @@ name: root class_name: LocalizationTable properties: Contents: - String: "[{\"key\":\"Ack\",\"example\":\"An exclamation of despair\",\"source\":\"Ack!\",\"values\":{\"es\":\"¡Ay!\"}}]" + String: "[{\"example\":\"An exclamation of despair\",\"key\":\"Ack\",\"source\":\"Ack!\",\"values\":{\"es\":\"¡Ay!\"}}]" children: [] diff --git a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_with_meta.snap b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_with_meta.snap index f90eccc8..b02dee1c 100644 --- a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_with_meta.snap +++ b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_with_meta.snap @@ -20,5 +20,5 @@ name: foo class_name: LocalizationTable properties: Contents: - String: "[{\"key\":\"Ack\",\"example\":\"An exclamation of despair\",\"source\":\"Ack!\",\"values\":{\"es\":\"¡Ay!\"}}]" + String: "[{\"example\":\"An exclamation of despair\",\"key\":\"Ack\",\"source\":\"Ack!\",\"values\":{\"es\":\"¡Ay!\"}}]" children: []