From 7b113e58b8aea836550303b41293f3713e60b8da Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Thu, 12 Dec 2024 10:52:49 -0500 Subject: [PATCH] fix(mfe): multiple versions (#9595) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Description We had a bug where if there were multiple MFE configs that contained the same application we would defer to config name ordering for which config would get selected. This PR makes it so we now select the newest versioned configuration. ⚠️ Fix a bug I introduced in https://github.com/vercel/turborepo/pull/9582 where I cast the relative unix path to a basic `str` and I then misused it with `join_component`. ### Testing Instructions Updated and added unit tests. Tested on a repo with multiple overlapping configs with different versions. --- crates/turborepo-lib/src/microfrontends.rs | 176 +++++++++++++----- .../src/task_graph/visitor/command.rs | 6 +- crates/turborepo-microfrontends/src/lib.rs | 10 +- 3 files changed, 142 insertions(+), 50 deletions(-) diff --git a/crates/turborepo-lib/src/microfrontends.rs b/crates/turborepo-lib/src/microfrontends.rs index ac803f95c1eac..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}; @@ -14,11 +14,17 @@ 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>, + version: &'static str, + path: Option, +} + impl MicrofrontendsConfigs { /// Constructs a collection of configurations from disk pub fn from_disk( @@ -39,7 +45,6 @@ impl MicrofrontendsConfigs { ) -> Result, Error> { let PackageGraphResult { configs, - config_paths, missing_default_apps, unsupported_version, mfe_package, @@ -58,7 +63,6 @@ impl MicrofrontendsConfigs { Ok((!configs.is_empty()).then_some(Self { configs, - config_paths, mfe_package, })) } @@ -68,22 +72,24 @@ 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)?; - Some(path.as_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) } pub fn update_turbo_json( @@ -107,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()), @@ -138,34 +144,40 @@ 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 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 +188,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; @@ -204,17 +215,11 @@ 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(); - configs.insert(package_name.to_string(), tasks); + let mut info = ConfigInfo::new(&config); 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 +229,6 @@ impl PackageGraphResult { missing_default_apps.sort(); Ok(Self { configs, - config_paths, missing_default_apps, unsupported_version, mfe_package, @@ -236,6 +240,26 @@ impl PackageGraphResult { struct FindResult<'a> { dev: Option>, proxy: TaskId<'a>, + version: &'static str, +} + +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)] @@ -257,7 +281,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, version: "2", path: None }); )+ _map } @@ -266,6 +290,7 @@ mod test { struct PackageUpdateTest { package_name: &'static str, + version: &'static str, result: Option, } @@ -278,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), @@ -307,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, } @@ -325,14 +358,15 @@ 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"); + 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", "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)] @@ -340,13 +374,13 @@ 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!( - "mfe-config-pkg" => ["web#dev", "docs#dev"], - "mfe-web" => ["mfe-web#dev", "mfe-docs#serve"] + 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, - config_paths: HashMap::new(), mfe_package: None, }; assert_eq!( @@ -456,12 +490,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(), @@ -474,4 +508,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(); + 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-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); diff --git a/crates/turborepo-microfrontends/src/lib.rs b/crates/turborepo-microfrontends/src/lib.rs index 9f63ab5273d39..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; @@ -126,6 +125,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)> { @@ -180,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;