Changed workspaces filter should account for lockfile updates

[finn-orsini]

Describe the feature you'd like to request

TL;DR

Turbo --filter by changed workspaces should mark workspaces as changed if they have been impacted by a lockfile update.

Full Description

Currently, when filtering by changed workspaces, lockfile updates do not mark workspaces as changed. Example:

• git clone [email protected]:finn-orsini/turbo-lockfile-mvr.git
• cd turbo-lockfile-mvr
• git checkout changed-lockfile && yarn (branch diff)
• yarn build-filter-dry

The output of the above is:

$ turbo run build --dry=json --filter="[origin/main]"
{
"packages": [
  "//"
],
"tasks": []
}

The // token indicates that something in the root of the workspace changed, but does not tell us if this is a meaningful / build impacting change (i.e. a lockfile update), or a root change that will not impact any builds (a README update).

Lockfile updates directly impact application builds. Given a build system that relies on --filter to detect which applications need to build & deploy, breaking lockfile changes can currently be merged to main without proper validation. They will be discovered the next time an impacted app is changed, which results in developer confusion: Why are my changes breaking in this way, which seems unrelated to the changes on my branch? How did main get into this state?

Notes

• This is distinct from existing issues on if caching behavior should/shouldn't include the lockfile, and from adding the lockfile to globalDependencies or inputs in turbo.json. In this issue, we are only concerned with the --filter logic, not if the resulting tasks are cached or not.
• The example repo is contrived, but we saw this in our prod repo with a lockfile-only upgrade of our design system (using yarn upgrade-interactive) not marking any workspaces as changed when all should have been.

Describe the solution you'd like

Turbo filtering by changed workspaces should include workspaces impacted by a lockfile update.

Ideally the solution would be "smart" - that is, not all lockfile changes result in all workspaces building, only those directly impacted by the update.

This could potentially be implemented by using the same logic as the prune option, by comparing the minimal lockfile on each ref (i.e. origin/main and the current commit) for each workspace.

We expect that this behavior would be opt-in, at least to start.

Describe alternatives you've considered

Our current alternative is a naive solution - whenever the lockfile changes, we omit the --filter flag to build all workspaces. This could be built into turbo via a globalFilterDependencies option in turbo.json.

This method is not ideal. Instead of false negatives where apps are not being built that should be, we now have false positives where apps are built that didn't need to be.

[gsoltis]

I think this would be a great feature to have.

From an initial glance, some of the pieces we'd need to implement are:

• use git to get the lockfile at the fromRef here
• parse the lockfile, similar to how we do it here
• iterate the list of packages, and for each one check for equality on the resolved Subgraph

Some pieces to resolve:

• Do we need the old package.json files to properly prune the old lockfile? Or maybe we can consider any failure to get a subtree as a change? We may need to make the subgraph resolution more robust to handle these cases.

[nathanhammond]

I think the current package.json resolutions should still work because in a lockfile-only update the specifiers should stay the same? @chris-olszewski would be the person who can confirm that hypothesis.

[finn-orsini]

@nathanhammond can you elaborate on that a bit? I'm not following yet. Specifically I'm not sure if you're saying there is existing logic this feature could draw from, or that you think this should already be working, or something else 😅

[nathanhammond]

No, it definitely doesn't work as is, I'm just thinking that the largest hurdle to accomplishing this task that could exist may not be that bad.

Each workspace has a package.json, if you update just the lock file the specifiers in package.json should be the same.

That specifier (e.g. foo@^1.0.3) is how we look things up in the lock file and figure out how it got resolved.

I think we could just add those resolutions to our cache key.

[nathanhammond]

One more note: I believe that we should treat this as a bug.

The root lockfile updated jest. The UI package didn't change but the resolution for its jest specifiers did change.

In other words, following the setup in @finn-orsini's report and then running turbo run build --dry=json --filter="web...[origin/main]" should return "packages": ["//", "web"]. It's a correctness issue that it does not.

/cc @tknickman this is impacting all turbo-ignore invocations right now. Update just the lockfile and redeploy? Ignored.

[tknickman]

https://github.com/vercel/turborepo/pull/2211 implements the naive approach (for now) and will return all workspaces when changes are detected to:

1. turbo.json
2. root package.json
3. package manager lock file

[attila]

Yes, that change in #2211 now triggers a full build on all our 100+ workspaces every time anything changes in package-lock.json in the root, regardless of relevance. The naive approach could use some refinement 😅

[arantespp]

#2211 implements the naive approach (for now) and will return all workspaces when changes are detected to:

1. turbo.json
2. root package.json
3. package manager lock file

How to ignore these changes? Because I want to run only modified packages when I'm in a development environment, and I don't want a full build and deployment.

[yarinsa]

Hey having the same issue and figured it could be a nice contribution to turbo. See my issue which was resolved by #2211 .

I think that the expected logic should be:

1. Collect all yarn lock changes since commit X
2. Filter all the dependencies that their final resolutions changed comparing to the yarn lock before commit X
3. Check all the packages in the workspace that include those dependencies
4. Return number 3 as the output of filter

Questions:

1. What do you guys think about this? does this make sense? am I missing something?
2. Anyone who already contributed are familiar with the code that can direct me to the right area?

@tknickman Maybe I can take it with you as a side community contribution?

[tknickman]

Yea that sounds good, @chris-olszewski tagging you in here too for the lockfile portion

[attila]

@yarinsa it looks great but ideally this logic should be supported on all lockfiles, not only yarn. We for example just use npm workspaces so package-lock.json support would be desirable here.

[yarinsa]

@yarinsa it looks great but ideally this logic should be supported on all lockfiles, not only yarn. We for example just use npm workspaces so package-lock.json support would be desirable here.

Finally got to work on that logic last week, but I spent a day trying to setup the env. If I can find help with that I assume implementing this for all lock won't be much different

[chris-olszewski]

Hi all, I was able to set aside some time last week to put together #3250 which should be a complete implementation. I still need to do quite a bit of testing before I'll feel comfortable shipping it, but I think most of the heavy lifting is done.

[fjeldstad]

Hi all, I was able to set aside some time last week to put together #3250 which should be a complete implementation. I still need to do quite a bit of testing before I'll feel comfortable shipping it, but I think most of the heavy lifting is done.

Since this was released a while back (1.7.3?), has this issue been resolved?

[attila]

Yes it's been working great for us, a life saver of not to rebuild everything every time on any lockfile change anymore. It was a real pain on our 140+ workspace npm repo, but now all is well 👏

Thank you for fixing this!

[finn-orsini]

Sorry for delay in closing, looks like this has been resolved. Thanks all!