From ffc5d44ad02a6dfdc6b0cfbd3acf5e4a1b3e376b Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Mon, 9 Dec 2024 13:38:57 -0500 Subject: [PATCH 1/5] chore(mfe): update unit tests to hit bug --- crates/turborepo-lib/src/microfrontends.rs | 64 ++++++++++++++++++++-- 1 file changed, 58 insertions(+), 6 deletions(-) diff --git a/crates/turborepo-lib/src/microfrontends.rs b/crates/turborepo-lib/src/microfrontends.rs index ac803f95c1eac..3e0701b454f9e 100644 --- a/crates/turborepo-lib/src/microfrontends.rs +++ b/crates/turborepo-lib/src/microfrontends.rs @@ -326,13 +326,13 @@ mod test { const NON_MFE_PKG: PackageUpdateTest = PackageUpdateTest::new("other-pkg"); const MFE_CONFIG_PKG: PackageUpdateTest = - PackageUpdateTest::new("mfe-config-pkg").proxy_only("mfe-config-pkg#proxy"); + PackageUpdateTest::new("z-config-pkg").proxy_only("z-config-pkg#proxy"); const MFE_CONFIG_PKG_DEV_TASK: PackageUpdateTest = - PackageUpdateTest::new("web").dev("web#dev", "mfe-config-pkg#proxy"); + PackageUpdateTest::new("web").dev("web#dev", "web#proxy"); const DEFAULT_APP_PROXY: PackageUpdateTest = - PackageUpdateTest::new("mfe-docs").dev("mfe-docs#serve", "mfe-web#proxy"); + PackageUpdateTest::new("docs").dev("docs#serve", "web#proxy"); const DEFAULT_APP_PROXY_AND_DEV: PackageUpdateTest = - PackageUpdateTest::new("mfe-web").dev("mfe-web#dev", "mfe-web#proxy"); + PackageUpdateTest::new("web").dev("web#dev", "web#proxy"); #[test_case(NON_MFE_PKG)] #[test_case(MFE_CONFIG_PKG)] @@ -341,8 +341,8 @@ mod test { #[test_case(DEFAULT_APP_PROXY_AND_DEV)] fn test_package_turbo_json_update(test: PackageUpdateTest) { let configs = mfe_configs!( - "mfe-config-pkg" => ["web#dev", "docs#dev"], - "mfe-web" => ["mfe-web#dev", "mfe-docs#serve"] + "z-config-pkg" => ["web#dev", "docs#dev"], + "web" => ["web#dev", "docs#serve"] ); let mfe = MicrofrontendsConfigs { configs, @@ -474,4 +474,56 @@ mod test { .unwrap(); assert_eq!(actual.global_deps, &["web/microfrontends.json".to_owned()]); } + + #[test] + fn test_v2_and_v1() { + let config_v2 = MFEConfig::from_str( + &serde_json::to_string_pretty(&json!({ + "version": "2", + "applications": { + "web": {}, + "docs": { + "development": { + "task": "serve" + } + } + } + })) + .unwrap(), + "something.txt", + ) + .unwrap(); + let config_v1 = MFEConfig::from_str( + &serde_json::to_string_pretty(&json!({ + "version": "1", + "applications": { + "web": {}, + "docs": { + "development": { + "task": "serve" + } + } + } + })) + .unwrap(), + "something.txt", + ) + .unwrap(); + let result = PackageGraphResult::new( + vec![ + ("web", Ok(Some(config_v2))), + ("docs", Ok(None)), + ("mfe-config", Ok(Some(config_v1))), + ] + .into_iter(), + ) + .unwrap(); + assert_eq!( + result.configs, + mfe_configs!( + "web" => ["web#dev", "docs#serve"], + "mfe-config" => ["web#dev", "docs#serve"] + ) + ) + } } From 9a6abea1b2b1c3b992a506eb51c3f5441e16d688 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Mon, 9 Dec 2024 14:17:27 -0500 Subject: [PATCH 2/5] chore(mfe): combine config info pieces --- crates/turborepo-lib/src/microfrontends.rs | 51 ++++++++++++---------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/crates/turborepo-lib/src/microfrontends.rs b/crates/turborepo-lib/src/microfrontends.rs index 3e0701b454f9e..aa3df482f3a53 100644 --- a/crates/turborepo-lib/src/microfrontends.rs +++ b/crates/turborepo-lib/src/microfrontends.rs @@ -14,11 +14,16 @@ use crate::{ #[derive(Debug, Clone)] pub struct MicrofrontendsConfigs { - configs: HashMap>>, - config_paths: HashMap, + configs: HashMap, mfe_package: Option<&'static str>, } +#[derive(Debug, Clone, Default, PartialEq)] +struct ConfigInfo { + tasks: HashSet>, + path: Option, +} + impl MicrofrontendsConfigs { /// Constructs a collection of configurations from disk pub fn from_disk( @@ -39,7 +44,6 @@ impl MicrofrontendsConfigs { ) -> Result, Error> { let PackageGraphResult { configs, - config_paths, missing_default_apps, unsupported_version, mfe_package, @@ -58,7 +62,6 @@ impl MicrofrontendsConfigs { Ok((!configs.is_empty()).then_some(Self { configs, - config_paths, mfe_package, })) } @@ -68,21 +71,23 @@ impl MicrofrontendsConfigs { } pub fn configs(&self) -> impl Iterator>)> { - self.configs.iter() + self.configs.iter().map(|(pkg, info)| (pkg, &info.tasks)) } pub fn get(&self, package_name: &str) -> Option<&HashSet>> { - self.configs.get(package_name) + let info = self.configs.get(package_name)?; + Some(&info.tasks) } pub fn task_has_mfe_proxy(&self, task_id: &TaskId) -> bool { self.configs .values() - .any(|dev_tasks| dev_tasks.contains(task_id)) + .any(|info| info.tasks.contains(task_id)) } pub fn config_filename(&self, package_name: &str) -> Option<&str> { - let path = self.config_paths.get(package_name)?; + let info = self.configs.get(package_name)?; + let path = info.path.as_ref()?; Some(path.as_str()) } @@ -155,17 +160,19 @@ impl MicrofrontendsConfigs { // Returns a list of repo relative paths to all MFE configurations fn configuration_file_paths(&self) -> Vec { - self.config_paths + self.configs .values() - .map(|config_path| config_path.as_str().to_string()) + .filter_map(|info| { + let path = info.path.as_ref()?; + Some(path.as_str().to_string()) + }) .collect() } } // Internal struct used to capture the results of checking the package graph struct PackageGraphResult { - configs: HashMap>>, - config_paths: HashMap, + configs: HashMap, missing_default_apps: Vec, unsupported_version: Vec<(String, String)>, mfe_package: Option<&'static str>, @@ -176,7 +183,6 @@ impl PackageGraphResult { packages: impl Iterator, Error>)>, ) -> Result { let mut configs = HashMap::new(); - let mut config_paths = HashMap::new(); let mut referenced_default_apps = HashSet::new(); let mut unsupported_version = Vec::new(); let mut mfe_package = None; @@ -211,10 +217,11 @@ impl PackageGraphResult { TaskId::new(application, dev_task).into_owned() }) .collect(); - configs.insert(package_name.to_string(), tasks); + let mut info = ConfigInfo { tasks, path: None }; if let Some(path) = config.path() { - config_paths.insert(package_name.to_string(), path.to_unix()); + info.path = Some(path.to_unix()); } + configs.insert(package_name.to_string(), info); } let default_apps_found = configs.keys().cloned().collect(); let mut missing_default_apps = referenced_default_apps @@ -224,7 +231,6 @@ impl PackageGraphResult { missing_default_apps.sort(); Ok(Self { configs, - config_paths, missing_default_apps, unsupported_version, mfe_package, @@ -257,7 +263,7 @@ mod test { for _dev_task in $dev_tasks.as_slice() { _dev_tasks.insert(crate::run::task_id::TaskName::from(*_dev_task).task_id().unwrap().into_owned()); } - _map.insert($config_owner.to_string(), _dev_tasks); + _map.insert($config_owner.to_string(), ConfigInfo { tasks: _dev_tasks, path: None }); )+ _map } @@ -346,7 +352,6 @@ mod test { ); let mfe = MicrofrontendsConfigs { configs, - config_paths: HashMap::new(), mfe_package: None, }; assert_eq!( @@ -456,12 +461,12 @@ mod test { #[test] fn test_configs_added_as_global_deps() { let configs = MicrofrontendsConfigs { - configs: vec![("web".to_owned(), Default::default())] - .into_iter() - .collect(), - config_paths: vec![( + configs: vec![( "web".to_owned(), - RelativeUnixPathBuf::new("web/microfrontends.json").unwrap(), + ConfigInfo { + path: Some(RelativeUnixPathBuf::new("web/microfrontends.json").unwrap()), + ..Default::default() + }, )] .into_iter() .collect(), From 6b7823a72c69b0f37575756a233b97c8c9d658ec Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Mon, 9 Dec 2024 14:28:35 -0500 Subject: [PATCH 3/5] chore(mfe): capture config version --- crates/turborepo-lib/src/microfrontends.rs | 45 ++++++++++++++-------- crates/turborepo-microfrontends/src/lib.rs | 7 ++++ 2 files changed, 36 insertions(+), 16 deletions(-) diff --git a/crates/turborepo-lib/src/microfrontends.rs b/crates/turborepo-lib/src/microfrontends.rs index aa3df482f3a53..0e2f28497742a 100644 --- a/crates/turborepo-lib/src/microfrontends.rs +++ b/crates/turborepo-lib/src/microfrontends.rs @@ -21,6 +21,7 @@ pub struct MicrofrontendsConfigs { #[derive(Debug, Clone, Default, PartialEq)] struct ConfigInfo { tasks: HashSet>, + version: &'static str, path: Option, } @@ -210,14 +211,7 @@ impl PackageGraphResult { else { continue; }; - let tasks = config - .development_tasks() - .map(|(application, options)| { - let dev_task = options.unwrap_or("dev"); - TaskId::new(application, dev_task).into_owned() - }) - .collect(); - let mut info = ConfigInfo { tasks, path: None }; + let mut info = ConfigInfo::new(&config); if let Some(path) = config.path() { info.path = Some(path.to_unix()); } @@ -244,6 +238,25 @@ struct FindResult<'a> { proxy: TaskId<'a>, } +impl ConfigInfo { + fn new(config: &MFEConfig) -> Self { + let tasks = config + .development_tasks() + .map(|(application, options)| { + let dev_task = options.unwrap_or("dev"); + TaskId::new(application, dev_task).into_owned() + }) + .collect(); + let version = config.version(); + + Self { + tasks, + version, + path: None, + } + } +} + #[cfg(test)] mod test { use serde_json::json; @@ -263,7 +276,7 @@ mod test { for _dev_task in $dev_tasks.as_slice() { _dev_tasks.insert(crate::run::task_id::TaskName::from(*_dev_task).task_id().unwrap().into_owned()); } - _map.insert($config_owner.to_string(), ConfigInfo { tasks: _dev_tasks, path: None }); + _map.insert($config_owner.to_string(), ConfigInfo { tasks: _dev_tasks, version: "2", path: None }); )+ _map } @@ -523,12 +536,12 @@ mod test { .into_iter(), ) .unwrap(); - assert_eq!( - result.configs, - mfe_configs!( - "web" => ["web#dev", "docs#serve"], - "mfe-config" => ["web#dev", "docs#serve"] - ) - ) + let mut expected = mfe_configs!( + "web" => ["web#dev", "docs#serve"], + "mfe-config" => ["web#dev", "docs#serve"] + ); + expected.get_mut("mfe-config").unwrap().version = "1"; + + assert_eq!(result.configs, expected,) } } diff --git a/crates/turborepo-microfrontends/src/lib.rs b/crates/turborepo-microfrontends/src/lib.rs index 9f63ab5273d39..6bb44b05ccc80 100644 --- a/crates/turborepo-microfrontends/src/lib.rs +++ b/crates/turborepo-microfrontends/src/lib.rs @@ -126,6 +126,13 @@ impl Config { Some(path) } + pub fn version(&self) -> &'static str { + match &self.inner { + ConfigInner::V1(_) => "1", + ConfigInner::V2(_) => "2", + } + } + fn load_v2_dir(dir: &AbsoluteSystemPath) -> Result, Error> { let load_config = |filename: &str| -> Option<(Result, AbsoluteSystemPathBuf)> { From 7d85482598e2bbcd3986d779e0360198f4c61e99 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Mon, 9 Dec 2024 14:48:09 -0500 Subject: [PATCH 4/5] fix(mfe): prefer higher versions of configs --- crates/turborepo-lib/src/microfrontends.rs | 30 +++++++++++++++++----- crates/turborepo-microfrontends/src/lib.rs | 3 --- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/crates/turborepo-lib/src/microfrontends.rs b/crates/turborepo-lib/src/microfrontends.rs index 0e2f28497742a..0aefbe86708e9 100644 --- a/crates/turborepo-lib/src/microfrontends.rs +++ b/crates/turborepo-lib/src/microfrontends.rs @@ -113,7 +113,7 @@ impl MicrofrontendsConfigs { // - contains the proxy task // - a member of one of the microfrontends // then we need to modify its task definitions - if let Some(FindResult { dev, proxy }) = self.package_turbo_json_update(package_name) { + if let Some(FindResult { dev, proxy, .. }) = self.package_turbo_json_update(package_name) { // We need to modify turbo.json, use default one if there isn't one present let mut turbo_json = turbo_json.or_else(|err| match err { config::Error::NoTurboJSON => Ok(TurboJson::default()), @@ -144,19 +144,23 @@ impl MicrofrontendsConfigs { &'a self, package_name: &PackageName, ) -> Option> { - self.configs().find_map(|(config, tasks)| { - let dev_task = tasks.iter().find_map(|task| { + let results = self.configs.iter().filter_map(|(config, info)| { + let dev_task = info.tasks.iter().find_map(|task| { (task.package() == package_name.as_str()).then(|| FindResult { dev: Some(task.as_borrowed()), proxy: TaskId::new(config, "proxy"), + version: info.version, }) }); let proxy_owner = (config.as_str() == package_name.as_str()).then(|| FindResult { dev: None, proxy: TaskId::new(config, "proxy"), + version: info.version, }); dev_task.or(proxy_owner) - }) + }); + // We invert the standard comparing order so higher versions are prioritized + results.sorted_by(|a, b| b.version.cmp(a.version)).next() } // Returns a list of repo relative paths to all MFE configurations @@ -236,6 +240,7 @@ impl PackageGraphResult { struct FindResult<'a> { dev: Option>, proxy: TaskId<'a>, + version: &'static str, } impl ConfigInfo { @@ -285,6 +290,7 @@ mod test { struct PackageUpdateTest { package_name: &'static str, + version: &'static str, result: Option, } @@ -297,10 +303,16 @@ mod test { pub const fn new(package_name: &'static str) -> Self { Self { package_name, + version: "2", result: None, } } + pub const fn v1(mut self) -> Self { + self.version = "1"; + self + } + pub const fn dev(mut self, dev: &'static str, proxy: &'static str) -> Self { self.result = Some(TestFindResult { dev: Some(dev), @@ -326,10 +338,12 @@ mod test { }) => Some(FindResult { dev: Some(Self::str_to_task(dev)), proxy: Self::str_to_task(proxy), + version: self.version, }), Some(TestFindResult { dev: None, proxy }) => Some(FindResult { dev: None, proxy: Self::str_to_task(proxy), + version: self.version, }), None => None, } @@ -344,8 +358,9 @@ mod test { } const NON_MFE_PKG: PackageUpdateTest = PackageUpdateTest::new("other-pkg"); - const MFE_CONFIG_PKG: PackageUpdateTest = - PackageUpdateTest::new("z-config-pkg").proxy_only("z-config-pkg#proxy"); + const MFE_CONFIG_PKG: PackageUpdateTest = PackageUpdateTest::new("z-config-pkg") + .v1() + .proxy_only("z-config-pkg#proxy"); const MFE_CONFIG_PKG_DEV_TASK: PackageUpdateTest = PackageUpdateTest::new("web").dev("web#dev", "web#proxy"); const DEFAULT_APP_PROXY: PackageUpdateTest = @@ -359,10 +374,11 @@ mod test { #[test_case(DEFAULT_APP_PROXY)] #[test_case(DEFAULT_APP_PROXY_AND_DEV)] fn test_package_turbo_json_update(test: PackageUpdateTest) { - let configs = mfe_configs!( + let mut configs = mfe_configs!( "z-config-pkg" => ["web#dev", "docs#dev"], "web" => ["web#dev", "docs#serve"] ); + configs.get_mut("z-config-pkg").unwrap().version = "1"; let mfe = MicrofrontendsConfigs { configs, mfe_package: None, diff --git a/crates/turborepo-microfrontends/src/lib.rs b/crates/turborepo-microfrontends/src/lib.rs index 6bb44b05ccc80..73b043683c279 100644 --- a/crates/turborepo-microfrontends/src/lib.rs +++ b/crates/turborepo-microfrontends/src/lib.rs @@ -1,4 +1,3 @@ -#![feature(assert_matches)] #![deny(clippy::all)] mod configv1; mod configv2; @@ -187,8 +186,6 @@ impl Config { #[cfg(test)] mod test { - use std::assert_matches::assert_matches; - use insta::assert_snapshot; use tempfile::TempDir; use test_case::test_case; From 8286fe59ffb6eca299f08e9a1ece2223c61f9578 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Mon, 9 Dec 2024 15:03:33 -0500 Subject: [PATCH 5/5] fix(mfe): use correct path type for config file --- crates/turborepo-lib/src/microfrontends.rs | 6 +++--- crates/turborepo-lib/src/task_graph/visitor/command.rs | 6 ++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/crates/turborepo-lib/src/microfrontends.rs b/crates/turborepo-lib/src/microfrontends.rs index 0aefbe86708e9..982f6aa20531d 100644 --- a/crates/turborepo-lib/src/microfrontends.rs +++ b/crates/turborepo-lib/src/microfrontends.rs @@ -2,7 +2,7 @@ use std::collections::{HashMap, HashSet}; use itertools::Itertools; use tracing::warn; -use turbopath::{AbsoluteSystemPath, RelativeUnixPathBuf}; +use turbopath::{AbsoluteSystemPath, RelativeUnixPath, RelativeUnixPathBuf}; use turborepo_microfrontends::{Config as MFEConfig, Error, MICROFRONTENDS_PACKAGES}; use turborepo_repository::package_graph::{PackageGraph, PackageName}; @@ -86,10 +86,10 @@ impl MicrofrontendsConfigs { .any(|info| info.tasks.contains(task_id)) } - pub fn config_filename(&self, package_name: &str) -> Option<&str> { + pub fn config_filename(&self, package_name: &str) -> Option<&RelativeUnixPath> { let info = self.configs.get(package_name)?; let path = info.path.as_ref()?; - Some(path.as_str()) + Some(path) } pub fn update_turbo_json( diff --git a/crates/turborepo-lib/src/task_graph/visitor/command.rs b/crates/turborepo-lib/src/task_graph/visitor/command.rs index 2eaf2d6f05d44..335f7040fe90b 100644 --- a/crates/turborepo-lib/src/task_graph/visitor/command.rs +++ b/crates/turborepo-lib/src/task_graph/visitor/command.rs @@ -211,7 +211,9 @@ impl<'a> CommandProvider for MicroFrontendProxyProvider<'a> { let mfe_config_filename = self.mfe_configs.config_filename(task_id.package()); return Err(Error::MissingMFEDependency { package: task_id.package().into(), - mfe_config_filename: mfe_config_filename.unwrap_or_default().to_owned(), + mfe_config_filename: mfe_config_filename + .map(|p| p.to_string()) + .unwrap_or_default(), }); } let local_apps = dev_tasks @@ -223,7 +225,7 @@ impl<'a> CommandProvider for MicroFrontendProxyProvider<'a> { .mfe_configs .config_filename(task_id.package()) .expect("every microfrontends default application should have configuration path"); - let mfe_path = package_dir.join_component(mfe_config_filename); + let mfe_path = self.repo_root.join_unix_path(mfe_config_filename); let mut args = vec!["proxy", mfe_path.as_str(), "--names"]; args.extend(local_apps);