feat(query): package changes reason #9240
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
8 Skipped Deployments
|
| $ echo "hello world" > packages/util/new.js | ||
|
|
||
| Validate that both `my-app` and `util` are affected | ||
| $ ${TURBO} query "query { affectedPackages { items { name reason { __typename } } } }" |
There was a problem hiding this comment.
What's with the __typename thing? Is that going to stay? Can it just be reason (and the response also not have the value nested?)
…ageChangeReason> to avoid storing duplicate packages
…f so, we don't mark lockfile changes as invalidating root package - Plumb through more change reasons
… changes, but i added a specific reason variant so tools can filter this out
NicholasLYang
force-pushed
the
nicholasyang/package-change-reason
branch
from
cdc2b50 to
35039ac
Compare
There was a problem hiding this comment.
Approving so we can get people testing this sooner vs later, but I think we'll want to change the reasons in a few ways:
- Return all of the reasons why a package is in scope
- Include all matching criteria for some reasons e.g. global file changes should mention all global files that changed and not just the first one
- Lockfile changes would be a lot more useful if they returned what packages changed instead of lockfile contents.
| @@ -23,19 +23,52 @@ pub enum LockfileChange { | |||
| WithContent(Vec<u8>), | |||
| } | |||
|
|
|||
| #[derive(Debug, PartialEq, Eq)] | |||
| #[derive(Debug, PartialEq, Eq, Hash, Clone)] | |||
| pub enum PackageChangeReason { | |||
There was a problem hiding this comment.
Most of these variants seem more focused on why a package is included in a filter rather than "what has changed". Is PackagePresenceReason maybe a more accurate name for this enum?
| /// Root task was run | ||
| RootTask { task: String }, | ||
| /// The lockfile changed and caused this package to be invalidated | ||
| LockfileChanged { previous_lockfile: String }, |
There was a problem hiding this comment.
What are we expecting consumers to do with the previous lockfile content?
| file: AnchoredSystemPathBuf, | ||
| }, | ||
| LockfileChangeDetectionFailed { | ||
| previous_lockfile: String, |
There was a problem hiding this comment.
Same question as above: What are we expecting consumers to do with the previous lockfile content?
| DefaultGlobalFileChanged, | ||
| LockfileChangeDetectionFailed, | ||
| DefaultGlobalFileChanged { | ||
| file: AnchoredSystemPathBuf, |
There was a problem hiding this comment.
I think it would be useful to return all changed global files instead of just the first one.
| previous_lockfile: String::from_utf8_lossy(&content) | ||
| .to_string(), |
There was a problem hiding this comment.
I think the far more useful bit of information here would be returning the diff between the transitive closure that gets calculated in get_changed_packages_from_lockfile
| error: impl std::error::Error, | ||
| from_ref: Option<String>, | ||
| to_ref: Option<String>, | ||
| ) -> Result<ChangedFilesResult, Error> { | ||
| warn!( | ||
| "unable to detect git range, assuming all files have changed: {}", | ||
| error | ||
| ); |
There was a problem hiding this comment.
Should this warning go up a level since we're no longer making this decision in this crate?
| .map(|(name, _)| name.to_owned()) | ||
| .map(|(name, _)| PackageChange { | ||
| name: name.to_owned(), | ||
| reason: PackageChangeReason::IncludedByFilter { |
There was a problem hiding this comment.
Maybe NoFilter is a more accurate variant for end users to understand why a package was in the run scope?
| @@ -97,7 +97,7 @@ impl<'a> GitChangeDetector for ScopeChangeDetector<'a> { | |||
| include_uncommitted: bool, | |||
| allow_unknown_objects: bool, | |||
| merge_base: bool, | |||
| ) -> Result<HashSet<PackageChange>, ResolutionError> { | |||
| ) -> Result<HashMap<PackageName, PackageChangeReason>, ResolutionError> { | |||
There was a problem hiding this comment.
Should we be returning HashMap<PackageName, HashSet<PackageChangeReason>> so we correctly communicate to users if there are multiple reasons why a package is in scope?
| } | ||
| } | ||
| } | ||
|
|
||
| let mut all_packages = HashSet::new(); | ||
| let mut all_packages = HashMap::new(); |
There was a problem hiding this comment.
Especially here I feel like it would be useful to indicate if there are multiple reasons a package was in scope and currently the reason is dependent on the ordering of the extend calls.
| fn merge_changed_packages( | ||
| changed_packages: &mut HashMap<PackageName, PackageChangeReason>, | ||
| new_changes: impl IntoIterator<Item = (PackageName, PackageChangeReason)>, | ||
| ) { | ||
| for (package, reason) in new_changes { | ||
| if !changed_packages.contains_key(&package) { | ||
| changed_packages.insert(package, reason); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
fn merge_changed_packages(
changed_packages: &mut HashMap<PackageName, HashSet<PackageChangeReason>>,
new_changes: impl IntoIterator<Item = (PackageName, PackageChangeReason)>,
) {
for (package, reason) in new_changes {
let mut reasons = changed_packages.entry(package).or_default();
reasons.insert(reason);
}
}
Description
Plumbing through reason for why a package changed for query. Adds a
reasonfield which is a union of the different reasons a package could be added.This can be reviewed commit by commit, although there is a little bit of churn around types and some behavior.
Testing Instructions
Added some tests to
affected.t