From 138f74268c5e9aad1cc8bc3116dc8862150b7255 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Fri, 6 Dec 2024 13:53:43 -0500 Subject: [PATCH] fix(mfe): factor in configs to hashes --- Cargo.lock | 1 + crates/turborepo-lib/src/microfrontends.rs | 70 ++++++-- crates/turborepo-microfrontends/Cargo.toml | 1 + crates/turborepo-microfrontends/src/lib.rs | 189 ++++++++++++++++----- 4 files changed, 202 insertions(+), 59 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 32e1a2c97ede5..dc60d2ae111ce 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6552,6 +6552,7 @@ dependencies = [ "serde", "serde_json", "tempfile", + "test-case", "thiserror", "turbopath", "turborepo-errors", diff --git a/crates/turborepo-lib/src/microfrontends.rs b/crates/turborepo-lib/src/microfrontends.rs index 48a6f96120d5b..ac803f95c1eac 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; +use turbopath::{AbsoluteSystemPath, RelativeUnixPathBuf}; use turborepo_microfrontends::{Config as MFEConfig, Error, MICROFRONTENDS_PACKAGES}; use turborepo_repository::package_graph::{PackageGraph, PackageName}; @@ -15,7 +15,7 @@ use crate::{ #[derive(Debug, Clone)] pub struct MicrofrontendsConfigs { configs: HashMap>>, - config_filenames: HashMap, + config_paths: HashMap, mfe_package: Option<&'static str>, } @@ -28,7 +28,7 @@ impl MicrofrontendsConfigs { Self::from_configs(package_graph.packages().map(|(name, info)| { ( name.as_str(), - MFEConfig::load_from_dir(&repo_root.resolve(info.package_path())), + MFEConfig::load_from_dir(repo_root, info.package_path()), ) })) } @@ -39,7 +39,7 @@ impl MicrofrontendsConfigs { ) -> Result, Error> { let PackageGraphResult { configs, - config_filenames, + config_paths, missing_default_apps, unsupported_version, mfe_package, @@ -58,7 +58,7 @@ impl MicrofrontendsConfigs { Ok((!configs.is_empty()).then_some(Self { configs, - config_filenames, + config_paths, mfe_package, })) } @@ -82,15 +82,27 @@ impl MicrofrontendsConfigs { } pub fn config_filename(&self, package_name: &str) -> Option<&str> { - let filename = self.config_filenames.get(package_name)?; - Some(filename.as_str()) + let path = self.config_paths.get(package_name)?; + Some(path.as_str()) } pub fn update_turbo_json( &self, package_name: &PackageName, - turbo_json: Result, + mut turbo_json: Result, ) -> Result { + // We add all of the microfrontend configurations as global dependencies so any + // changes will invalidate all tasks. + // TODO: Move this to only apply to packages that are part of the + // microfrontends. This will require us operating on task definitions + // instead of `turbo.json`s which currently isn't feasible. + if matches!(package_name, PackageName::Root) { + if let Ok(turbo_json) = &mut turbo_json { + turbo_json + .global_deps + .append(&mut self.configuration_file_paths()); + } + } // If package either // - contains the proxy task // - a member of one of the microfrontends @@ -140,12 +152,20 @@ impl MicrofrontendsConfigs { dev_task.or(proxy_owner) }) } + + // Returns a list of repo relative paths to all MFE configurations + fn configuration_file_paths(&self) -> Vec { + self.config_paths + .values() + .map(|config_path| config_path.as_str().to_string()) + .collect() + } } // Internal struct used to capture the results of checking the package graph struct PackageGraphResult { configs: HashMap>>, - config_filenames: HashMap, + config_paths: HashMap, missing_default_apps: Vec, unsupported_version: Vec<(String, String)>, mfe_package: Option<&'static str>, @@ -156,7 +176,7 @@ impl PackageGraphResult { packages: impl Iterator, Error>)>, ) -> Result { let mut configs = HashMap::new(); - let mut config_filenames = 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; @@ -192,7 +212,9 @@ impl PackageGraphResult { }) .collect(); configs.insert(package_name.to_string(), tasks); - config_filenames.insert(package_name.to_string(), config.filename().to_owned()); + if let Some(path) = config.path() { + config_paths.insert(package_name.to_string(), path.to_unix()); + } } let default_apps_found = configs.keys().cloned().collect(); let mut missing_default_apps = referenced_default_apps @@ -202,7 +224,7 @@ impl PackageGraphResult { missing_default_apps.sort(); Ok(Self { configs, - config_filenames, + config_paths, missing_default_apps, unsupported_version, mfe_package, @@ -324,7 +346,7 @@ mod test { ); let mfe = MicrofrontendsConfigs { configs, - config_filenames: HashMap::new(), + config_paths: HashMap::new(), mfe_package: None, }; assert_eq!( @@ -430,4 +452,26 @@ 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![( + "web".to_owned(), + RelativeUnixPathBuf::new("web/microfrontends.json").unwrap(), + )] + .into_iter() + .collect(), + mfe_package: None, + }; + + let turbo_json = TurboJson::default(); + let actual = configs + .update_turbo_json(&PackageName::Root, Ok(turbo_json)) + .unwrap(); + assert_eq!(actual.global_deps, &["web/microfrontends.json".to_owned()]); + } } diff --git a/crates/turborepo-microfrontends/Cargo.toml b/crates/turborepo-microfrontends/Cargo.toml index 209cd30805caf..5c2208a644781 100644 --- a/crates/turborepo-microfrontends/Cargo.toml +++ b/crates/turborepo-microfrontends/Cargo.toml @@ -20,6 +20,7 @@ turborepo-errors = { workspace = true } insta = { workspace = true } pretty_assertions = { workspace = true } tempfile = { workspace = true } +test-case = { workspace = true } [lints] workspace = true diff --git a/crates/turborepo-microfrontends/src/lib.rs b/crates/turborepo-microfrontends/src/lib.rs index 58581faf6e273..9f63ab5273d39 100644 --- a/crates/turborepo-microfrontends/src/lib.rs +++ b/crates/turborepo-microfrontends/src/lib.rs @@ -11,7 +11,9 @@ use biome_json_parser::JsonParserOptions; use configv1::ConfigV1; use configv2::ConfigV2; pub use error::Error; -use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf}; +use turbopath::{ + AbsoluteSystemPath, AbsoluteSystemPathBuf, AnchoredSystemPath, AnchoredSystemPathBuf, +}; /// Currently the default path for a package that provides a configuration. /// @@ -34,6 +36,7 @@ pub const SUPPORTED_VERSIONS: &[&str] = ["1", "2"].as_slice(); pub struct Config { inner: ConfigInner, filename: String, + path: Option, } #[derive(Debug, PartialEq, Eq)] @@ -53,12 +56,22 @@ impl Config { Ok(Some(config)) } - pub fn load_from_dir(dir: &AbsoluteSystemPath) -> Result, Error> { - if let Some(config) = Self::load_v2_dir(dir)? { + /// Attempts to load a configuration file from the given directory + /// Returns `Ok(None)` if no configuration is found in the directory + pub fn load_from_dir( + repo_root: &AbsoluteSystemPath, + package_dir: &AnchoredSystemPath, + ) -> Result, Error> { + let absolute_dir = repo_root.resolve(package_dir); + let mut config = if let Some(config) = Self::load_v2_dir(&absolute_dir)? { Ok(Some(config)) } else { - Self::load_v1_dir(dir) + Self::load_v1_dir(&absolute_dir) + }; + if let Ok(Some(config)) = &mut config { + config.set_path(package_dir); } + config } pub fn from_str(input: &str, source: &str) -> Result { @@ -92,6 +105,7 @@ impl Config { Ok(Self { inner, filename: source.to_owned(), + path: None, }) } @@ -107,6 +121,11 @@ impl Config { &self.filename } + pub fn path(&self) -> Option<&AnchoredSystemPath> { + let path = self.path.as_deref()?; + Some(path) + } + fn load_v2_dir(dir: &AbsoluteSystemPath) -> Result, Error> { let load_config = |filename: &str| -> Option<(Result, AbsoluteSystemPathBuf)> { @@ -129,6 +148,7 @@ impl Config { .file_name() .expect("microfrontends config should not be root") .to_owned(), + path: None, }), configv2::ParseResult::Reference(default_app) => Err(Error::ChildConfig { reference: default_app, @@ -147,9 +167,15 @@ impl Config { .map(|config_v1| Self { inner: ConfigInner::V1(config_v1), filename: DEFAULT_MICROFRONTENDS_CONFIG_V1.to_owned(), + path: None, }) .map(Some) } + + /// Sets the path the configuration was loaded from + pub fn set_path(&mut self, dir: &AnchoredSystemPath) { + self.path = Some(dir.join_component(&self.filename)); + } } #[cfg(test)] @@ -158,6 +184,7 @@ mod test { use insta::assert_snapshot; use tempfile::TempDir; + use test_case::test_case; use super::*; @@ -177,68 +204,138 @@ mod test { fn add_v1_config(dir: &AbsoluteSystemPath) -> Result<(), std::io::Error> { let path = dir.join_component(DEFAULT_MICROFRONTENDS_CONFIG_V1); + path.ensure_dir()?; path.create_with_contents(r#"{"version": "1", "applications": {"web": {"development": {"task": "serve"}}, "docs": {}}}"#) } fn add_v2_config(dir: &AbsoluteSystemPath) -> Result<(), std::io::Error> { let path = dir.join_component(DEFAULT_MICROFRONTENDS_CONFIG_V2); + path.ensure_dir()?; path.create_with_contents(r#"{"version": "2", "applications": {"web": {"development": {"task": "serve"}}, "docs": {}}}"#) } fn add_v2_alt_config(dir: &AbsoluteSystemPath) -> Result<(), std::io::Error> { let path = dir.join_component(DEFAULT_MICROFRONTENDS_CONFIG_V2_ALT); + path.ensure_dir()?; path.create_with_contents(r#"{"version": "2", "applications": {"web": {"development": {"task": "serve"}}, "docs": {}}}"#) } - #[test] - fn test_load_dir_v1() { - let dir = TempDir::new().unwrap(); - let path = AbsoluteSystemPath::new(dir.path().to_str().unwrap()).unwrap(); - add_v1_config(path).unwrap(); - let config = Config::load_from_dir(path) - .unwrap() - .map(|config| config.inner); - assert_matches!(config, Some(ConfigInner::V1(_))); + struct LoadDirTest { + has_v1: bool, + has_v2: bool, + has_alt_v2: bool, + pkg_dir: &'static str, + expected_version: Option, + expected_filename: Option<&'static str>, } - #[test] - fn test_load_dir_v2() { - let dir = TempDir::new().unwrap(); - let path = AbsoluteSystemPath::new(dir.path().to_str().unwrap()).unwrap(); - add_v2_config(path).unwrap(); - let config = Config::load_from_dir(path) - .unwrap() - .map(|config| config.inner); - assert_matches!(config, Some(ConfigInner::V2(_))); + #[derive(Debug, Clone, Copy, PartialEq, Eq)] + enum FoundConfig { + V1, + V2, } - #[test] - fn test_load_dir_both() { - let dir = TempDir::new().unwrap(); - let path = AbsoluteSystemPath::new(dir.path().to_str().unwrap()).unwrap(); - add_v1_config(path).unwrap(); - add_v2_config(path).unwrap(); - let config = Config::load_from_dir(path) - .unwrap() - .map(|config| config.inner); - assert_matches!(config, Some(ConfigInner::V2(_))); - } + impl LoadDirTest { + pub const fn new(pkg_dir: &'static str) -> Self { + Self { + pkg_dir, + has_v1: false, + has_v2: false, + has_alt_v2: false, + expected_version: None, + expected_filename: None, + } + } - #[test] - fn test_load_dir_v2_alt() { - let dir = TempDir::new().unwrap(); - let path = AbsoluteSystemPath::new(dir.path().to_str().unwrap()).unwrap(); - add_v2_alt_config(path).unwrap(); - let config = Config::load_from_dir(path) - .unwrap() - .map(|config| config.inner); - assert_matches!(config, Some(ConfigInner::V2(_))); + pub const fn has_v1(mut self) -> Self { + self.has_v1 = true; + self + } + + pub const fn has_v2(mut self) -> Self { + self.has_v2 = true; + self + } + + pub const fn has_alt_v2(mut self) -> Self { + self.has_alt_v2 = true; + self + } + + pub const fn expects_v1(mut self) -> Self { + self.expected_version = Some(FoundConfig::V1); + self + } + + pub const fn expects_v2(mut self) -> Self { + self.expected_version = Some(FoundConfig::V2); + self + } + + pub const fn with_filename(mut self, filename: &'static str) -> Self { + self.expected_filename = Some(filename); + self + } + + pub fn expected_path(&self) -> Option { + let filename = self.expected_filename?; + Some( + AnchoredSystemPath::new(self.pkg_dir) + .unwrap() + .join_component(filename), + ) + } } - #[test] - fn test_load_dir_none() { + const LOAD_V1: LoadDirTest = LoadDirTest::new("web") + .has_v1() + .expects_v1() + .with_filename(DEFAULT_MICROFRONTENDS_CONFIG_V1); + + const LOAD_V2: LoadDirTest = LoadDirTest::new("web") + .has_v2() + .expects_v2() + .with_filename(DEFAULT_MICROFRONTENDS_CONFIG_V2); + + const LOAD_BOTH: LoadDirTest = LoadDirTest::new("web") + .has_v1() + .has_v2() + .expects_v2() + .with_filename(DEFAULT_MICROFRONTENDS_CONFIG_V2); + + const LOAD_V2_ALT: LoadDirTest = LoadDirTest::new("web") + .has_alt_v2() + .expects_v2() + .with_filename(DEFAULT_MICROFRONTENDS_CONFIG_V2_ALT); + + const LOAD_NONE: LoadDirTest = LoadDirTest::new("web"); + #[test_case(LOAD_V1)] + #[test_case(LOAD_V2)] + #[test_case(LOAD_BOTH)] + #[test_case(LOAD_V2_ALT)] + #[test_case(LOAD_NONE)] + fn test_load_dir(case: LoadDirTest) { let dir = TempDir::new().unwrap(); - let path = AbsoluteSystemPath::new(dir.path().to_str().unwrap()).unwrap(); - assert!(Config::load_from_dir(path).unwrap().is_none()); + let repo_root = AbsoluteSystemPath::new(dir.path().to_str().unwrap()).unwrap(); + let pkg_dir = AnchoredSystemPath::new(case.pkg_dir).unwrap(); + let pkg_path = repo_root.resolve(pkg_dir); + if case.has_v1 { + add_v1_config(&pkg_path).unwrap(); + } + if case.has_v2 { + add_v2_config(&pkg_path).unwrap(); + } + if case.has_alt_v2 { + add_v2_alt_config(&pkg_path).unwrap(); + } + + let config = Config::load_from_dir(repo_root, pkg_dir).unwrap(); + let actual_version = config.as_ref().map(|config| match &config.inner { + ConfigInner::V1(_) => FoundConfig::V1, + ConfigInner::V2(_) => FoundConfig::V2, + }); + let actual_path = config.as_ref().and_then(|config| config.path()); + assert_eq!(actual_version, case.expected_version); + assert_eq!(actual_path, case.expected_path().as_deref()); } }