Skip to content
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

fix: affected_packages's optimization flow #9950

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dimitropoulos
Copy link
Contributor

Description

This PR addresses the affected_packages API and its handling of the optimization for lockfiles. It should not change the current behaviors without this optimization.

Testing Instructions

In Progress...

Copy link

vercel bot commented Feb 13, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
examples-basic-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 25, 2025 1:03pm
examples-designsystem-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 25, 2025 1:03pm
examples-gatsby-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 25, 2025 1:03pm
examples-kitchensink-blog ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 25, 2025 1:03pm
examples-native-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 25, 2025 1:03pm
examples-nonmonorepo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 25, 2025 1:03pm
examples-svelte-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 25, 2025 1:03pm
examples-tailwind-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 25, 2025 1:03pm
examples-vite-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 25, 2025 1:03pm

@@ -70,23 +69,21 @@ impl<'a> ScopeChangeDetector<'a> {
&lockfile_path,
) {
debug!("lockfile did not change");
return None;
Copy link
Contributor Author

@dimitropoulos dimitropoulos Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this appears to have been sorta incorrect because the None state was intended to mean "there was a change but we don't know what it was" which maps, today, to LockfileContents::Unknown. But, as you can see here, it's being used in the codepath where LockfileContents::Unchanged is correct. Ultimately, it shouldn't be a behavioral change, though, because the codepaths have the same end behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't "there was a change but we don't know what it was" Some(None)? And None is "no change"

/// This describes the state of a change to a lockfile.
pub enum LockfileContents {
/// We know the lockfile did not change
Unchanged,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although at first I thought this might not be needed because we have the list of files and can simply see that the lockfile wasn't in there, it's used in change_detector with get_lockfile_contents.

dimitropoulos added a commit that referenced this pull request Feb 13, 2025
### Description

In #9950 (comment)
it was observed that `bun.lock` is missing from this list.

I started by exposing and then consuming the constants right from the
source (d9ea60f) and then added
`bun.lock` to the list in (c5840db)

### Testing Instructions

`bun.lock` should now invalidate. With
#9783 being released, this is
expected behavior.
joshnuss pushed a commit to joshnuss/turborepo that referenced this pull request Feb 15, 2025
### Description

In vercel#9950 (comment)
it was observed that `bun.lock` is missing from this list.

I started by exposing and then consuming the constants right from the
source (d9ea60f) and then added
`bun.lock` to the list in (c5840db)

### Testing Instructions

`bun.lock` should now invalidate. With
vercel#9783 being released, this is
expected behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants