New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(turborepo): Package Detection #7549
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
0c030cf
Factored out package detection into a trait with different implementa…
NicholasLYang e2edc50
Removed global deps from change mapper and put into package detector.…
NicholasLYang 045df53
PR feedback
NicholasLYang 276a632
Adding back global file detection because apparently we don't treat r…
NicholasLYang d1a40c4
Refactoring package mapping to use a PackageDetection struct
NicholasLYang 8bf8215
pulling out global deps into a different package detector, so we can
NicholasLYang 4233065
Removing change post-rebase
NicholasLYang c22d2ec
Fix call site and comment
NicholasLYang 3386cd3
Tidying up
NicholasLYang c3f48bb
Fixing tests
NicholasLYang 1b9de4d
Fix test
NicholasLYang fdbbae6
Update crates/turborepo-lib/src/global_deps_package_detector.rs
NicholasLYang b191cb3
PR feedback
NicholasLYang c4ca9e2
PR feedback
NicholasLYang fb4f85b
Fix up names
NicholasLYang File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
142 changes: 142 additions & 0 deletions
142
crates/turborepo-lib/src/global_deps_package_detector.rs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,142 @@ | ||
use thiserror::Error; | ||
use turbopath::AnchoredSystemPath; | ||
use turborepo_repository::{ | ||
change_mapper::{DefaultPackageDetector, PackageChangeMapper, PackageMapping}, | ||
package_graph::{PackageGraph, WorkspacePackage}, | ||
}; | ||
use wax::{BuildError, Program}; | ||
|
||
#[derive(Error, Debug)] | ||
pub enum Error { | ||
#[error(transparent)] | ||
InvalidFilter(#[from] BuildError), | ||
} | ||
|
||
/// A package detector that uses a global deps list to determine | ||
/// if a file should cause all packages to be marked as changed. | ||
/// This is less conservative than the `DefaultPackageDetector` | ||
/// which assumes that any changed file that is not in a package | ||
/// changes all packages. Since we have a list of global deps, | ||
/// we can check against that and avoid invalidating in unnecessary cases. | ||
pub struct GlobalDepsPackageDetector<'a> { | ||
pkg_dep_graph: &'a PackageGraph, | ||
global_deps_matcher: wax::Any<'a>, | ||
} | ||
|
||
impl<'a> GlobalDepsPackageDetector<'a> { | ||
pub fn new<S: wax::Pattern<'a>, I: Iterator<Item = S>>( | ||
pkg_dep_graph: &'a PackageGraph, | ||
global_deps: I, | ||
) -> Result<Self, Error> { | ||
let global_deps_matcher = wax::any(global_deps)?; | ||
|
||
Ok(Self { | ||
pkg_dep_graph, | ||
global_deps_matcher, | ||
}) | ||
} | ||
} | ||
|
||
impl<'a> PackageChangeMapper for GlobalDepsPackageDetector<'a> { | ||
fn detect_package(&self, path: &AnchoredSystemPath) -> PackageMapping { | ||
match DefaultPackageDetector::new(self.pkg_dep_graph).detect_package(path) { | ||
// Since `DefaultPackageDetector` is overly conservative, we can check here if | ||
// the path is actually in globalDeps and if not, return it as | ||
// PackageDetection::Package(WorkspacePackage::root()). | ||
PackageMapping::All => { | ||
let cleaned_path = path.clean(); | ||
let in_global_deps = self.global_deps_matcher.is_match(cleaned_path.as_str()); | ||
|
||
if in_global_deps { | ||
PackageMapping::All | ||
} else { | ||
PackageMapping::Package(WorkspacePackage::root()) | ||
} | ||
} | ||
result => result, | ||
} | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use tempfile::tempdir; | ||
use turbopath::{AbsoluteSystemPath, AnchoredSystemPathBuf}; | ||
use turborepo_repository::{ | ||
change_mapper::{ChangeMapper, DefaultPackageDetector, PackageChanges}, | ||
discovery, | ||
discovery::PackageDiscovery, | ||
package_graph::{PackageGraphBuilder, WorkspacePackage}, | ||
package_json::PackageJson, | ||
}; | ||
|
||
use super::GlobalDepsPackageDetector; | ||
|
||
#[allow(dead_code)] | ||
pub struct MockDiscovery; | ||
|
||
impl PackageDiscovery for MockDiscovery { | ||
async fn discover_packages( | ||
&self, | ||
) -> Result<discovery::DiscoveryResponse, discovery::Error> { | ||
Ok(discovery::DiscoveryResponse { | ||
package_manager: turborepo_repository::package_manager::PackageManager::Npm, | ||
workspaces: vec![], | ||
}) | ||
} | ||
|
||
async fn discover_packages_blocking( | ||
&self, | ||
) -> Result<discovery::DiscoveryResponse, discovery::Error> { | ||
self.discover_packages().await | ||
} | ||
} | ||
|
||
#[tokio::test] | ||
async fn test_different_package_detectors() -> Result<(), anyhow::Error> { | ||
let repo_root = tempdir()?; | ||
let root_package_json = PackageJson::default(); | ||
|
||
let pkg_graph = PackageGraphBuilder::new( | ||
AbsoluteSystemPath::from_std_path(repo_root.path())?, | ||
root_package_json, | ||
) | ||
.with_package_discovery(MockDiscovery) | ||
.build() | ||
.await?; | ||
|
||
let default_package_detector = DefaultPackageDetector::new(&pkg_graph); | ||
let change_mapper = ChangeMapper::new(&pkg_graph, vec![], default_package_detector); | ||
|
||
let package_changes = change_mapper.changed_packages( | ||
[AnchoredSystemPathBuf::from_raw("README.md")?] | ||
.into_iter() | ||
.collect(), | ||
None, | ||
)?; | ||
|
||
// We should return All because we don't have global deps and | ||
// therefore must be conservative about changes | ||
assert_eq!(package_changes, PackageChanges::All); | ||
|
||
let turbo_package_detector = | ||
GlobalDepsPackageDetector::new(&pkg_graph, std::iter::empty::<&str>())?; | ||
let change_mapper = ChangeMapper::new(&pkg_graph, vec![], turbo_package_detector); | ||
|
||
let package_changes = change_mapper.changed_packages( | ||
[AnchoredSystemPathBuf::from_raw("README.md")?] | ||
.into_iter() | ||
.collect(), | ||
None, | ||
)?; | ||
|
||
// We only get a root workspace change since we have global deps specified and | ||
// README.md is not one of them | ||
assert_eq!( | ||
package_changes, | ||
PackageChanges::Some([WorkspacePackage::root()].into_iter().collect()) | ||
); | ||
|
||
Ok(()) | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,6 @@ pub fn resolve_packages( | |
pkg_inference, | ||
scm, | ||
root_turbo_json, | ||
) | ||
)? | ||
.resolve(&opts.get_filters()) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having trouble tracing this code. Does the
global_deps_matcher
always get the default global deps ofpackage.json
/turbo.json
added to it? Otherwise we'll incorrectly map apackage.json
change just be a root package change instead of a all change.Or I'm misunderstanding these changes completely and this codepath isn't hit in that scenario.
Edit: I was misunderstanding, default global deps are checked at
ChangeMapper
which uses a package detector if a default global change isn't detected