Skip to content

Commit 31fc656

Browse files
fix(repository): still assume files not belonging to a package result in a global change (#10047)
### Description Make sure we keep as much of the default behavior as possible. We now have a nesting doll sorta structure for change mappers: ``` Most conservative > Least Conservative Default > DefaultWLockfile > Global ```
1 parent df15948 commit 31fc656

File tree

4 files changed

+69
-31
lines changed

4 files changed

+69
-31
lines changed

crates/turborepo-repository/src/change_mapper/mod.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@ use std::{
77
};
88

99
pub use package::{
10-
DefaultPackageChangeMapper, Error, GlobalDepsPackageChangeMapper, PackageChangeMapper,
11-
PackageMapping,
10+
DefaultPackageChangeMapper, DefaultPackageChangeMapperWithLockfile, Error,
11+
GlobalDepsPackageChangeMapper, PackageChangeMapper, PackageMapping,
1212
};
1313
use tracing::debug;
1414
use turbopath::{AbsoluteSystemPath, AnchoredSystemPathBuf};

crates/turborepo-repository/src/change_mapper/package.rs

+41-21
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,43 @@ impl PackageChangeMapper for DefaultPackageChangeMapper<'_> {
8686
}
8787
}
8888

89+
pub struct DefaultPackageChangeMapperWithLockfile<'a> {
90+
base: DefaultPackageChangeMapper<'a>,
91+
}
92+
93+
impl<'a> DefaultPackageChangeMapperWithLockfile<'a> {
94+
pub fn new(pkg_dep_graph: &'a PackageGraph) -> Self {
95+
Self {
96+
base: DefaultPackageChangeMapper::new(pkg_dep_graph),
97+
}
98+
}
99+
}
100+
101+
impl PackageChangeMapper for DefaultPackageChangeMapperWithLockfile<'_> {
102+
fn detect_package(&self, path: &AnchoredSystemPath) -> PackageMapping {
103+
// If we have a lockfile change, we consider this as a root package change,
104+
// since there's a chance that the root package uses a workspace package
105+
// dependency (this is cursed behavior but sadly possible). There's a chance
106+
// that we can make this more accurate by checking which package
107+
// manager, since not all package managers may permit root pulling from
108+
// workspace package dependencies
109+
if PackageManager::supported_managers()
110+
.iter()
111+
.any(|pm| pm.lockfile_name() == path.as_str())
112+
{
113+
PackageMapping::Package((
114+
WorkspacePackage {
115+
name: PackageName::Root,
116+
path: AnchoredSystemPathBuf::from_raw("").unwrap(),
117+
},
118+
PackageInclusionReason::ConservativeRootLockfileChanged,
119+
))
120+
} else {
121+
self.base.detect_package(path)
122+
}
123+
}
124+
}
125+
89126
#[derive(Error, Debug)]
90127
pub enum Error {
91128
#[error(transparent)]
@@ -101,7 +138,7 @@ pub enum Error {
101138
/// changes all packages. Since we have a list of global deps,
102139
/// we can check against that and avoid invalidating in unnecessary cases.
103140
pub struct GlobalDepsPackageChangeMapper<'a> {
104-
pkg_dep_graph: &'a PackageGraph,
141+
base: DefaultPackageChangeMapperWithLockfile<'a>,
105142
global_deps_matcher: wax::Any<'a>,
106143
}
107144

@@ -110,36 +147,19 @@ impl<'a> GlobalDepsPackageChangeMapper<'a> {
110147
pkg_dep_graph: &'a PackageGraph,
111148
global_deps: I,
112149
) -> Result<Self, Error> {
150+
let base = DefaultPackageChangeMapperWithLockfile::new(pkg_dep_graph);
113151
let global_deps_matcher = wax::any(global_deps)?;
114152

115153
Ok(Self {
116-
pkg_dep_graph,
154+
base,
117155
global_deps_matcher,
118156
})
119157
}
120158
}
121159

122160
impl PackageChangeMapper for GlobalDepsPackageChangeMapper<'_> {
123161
fn detect_package(&self, path: &AnchoredSystemPath) -> PackageMapping {
124-
// If we have a lockfile change, we consider this as a root package change,
125-
// since there's a chance that the root package uses a workspace package
126-
// dependency (this is cursed behavior but sadly possible). There's a chance
127-
// that we can make this more accurate by checking which package
128-
// manager, since not all package managers may permit root pulling from
129-
// workspace package dependencies
130-
if PackageManager::supported_managers()
131-
.iter()
132-
.any(|pm| pm.lockfile_name() == path.as_str())
133-
{
134-
return PackageMapping::Package((
135-
WorkspacePackage {
136-
name: PackageName::Root,
137-
path: AnchoredSystemPathBuf::from_raw("").unwrap(),
138-
},
139-
PackageInclusionReason::ConservativeRootLockfileChanged,
140-
));
141-
}
142-
match DefaultPackageChangeMapper::new(self.pkg_dep_graph).detect_package(path) {
162+
match self.base.detect_package(path) {
143163
// Since `DefaultPackageChangeMapper` is overly conservative, we can check here if
144164
// the path is actually in globalDeps and if not, return it as
145165
// PackageDetection::Package(WorkspacePackage::root()).

packages/turbo-repository/__tests__/affected-packages.test.ts

+23
Original file line numberDiff line numberDiff line change
@@ -68,5 +68,28 @@ describe("affectedPackages", () => {
6868
workspace.affectedPackages(["pnpm-lock.yaml"], null, true)
6969
);
7070
});
71+
72+
it("still considers root file changes as global", async () => {
73+
const dir = path.resolve(__dirname, "./fixtures/monorepo");
74+
const workspace = await Workspace.find(dir);
75+
76+
const reduced: PackageReduced[] = (
77+
await workspace.affectedPackages(
78+
["file-we-do-not-understand.txt"],
79+
"HEAD",
80+
true
81+
)
82+
).map((pkg) => {
83+
return {
84+
name: pkg.name,
85+
relativePath: pkg.relativePath,
86+
};
87+
});
88+
89+
assert.deepEqual(reduced, [
90+
{ name: "app-a", relativePath: "apps/app" },
91+
{ name: "ui", relativePath: "packages/ui" },
92+
]);
93+
});
7194
});
7295
});

packages/turbo-repository/rust/src/lib.rs

+3-8
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@ use tracing::debug;
1010
use turbopath::{AbsoluteSystemPath, AnchoredSystemPath, AnchoredSystemPathBuf};
1111
use turborepo_repository::{
1212
change_mapper::{
13-
ChangeMapper, DefaultPackageChangeMapper, GlobalDepsPackageChangeMapper, LockfileContents,
14-
PackageChanges,
13+
ChangeMapper, DefaultPackageChangeMapper, DefaultPackageChangeMapperWithLockfile,
14+
LockfileContents, PackageChanges,
1515
},
1616
inference::RepoState as WorkspaceState,
1717
package_graph::{PackageGraph, PackageName, PackageNode, WorkspacePackage, ROOT_PKG_NAME},
@@ -246,12 +246,7 @@ impl Workspace {
246246
// Create a ChangeMapper with no ignore patterns
247247
let change_detector = comparison
248248
.is_some()
249-
.then(|| {
250-
Either::Left(
251-
GlobalDepsPackageChangeMapper::new(&self.graph, std::iter::empty::<&str>())
252-
.unwrap(),
253-
)
254-
})
249+
.then(|| Either::Left(DefaultPackageChangeMapperWithLockfile::new(&self.graph)))
255250
.unwrap_or_else(|| Either::Right(DefaultPackageChangeMapper::new(&self.graph)));
256251
let mapper = ChangeMapper::new(&self.graph, vec![], change_detector);
257252

0 commit comments

Comments
 (0)