fix(turbo): sort dependents and dependencies during normalization #7018
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
8 Ignored Deployments
|
🟢 Turbopack Benchmark CI successful 🟢Thanks |
🟢 CI successful 🟢Thanks |
| .cloned() | ||
| .map(SinglePackageTaskSummary::from) | ||
| .collect::<Vec<_>>(); | ||
| tasks.sort_by(|t1, t2| t1.task_id.cmp(&t2.task_id)); |
There was a problem hiding this comment.
Removing extra sort() because tasks are sorted during normalization: https://github.com/vercel/turbo/blob/1dd2e93e84e08872f67943804a6600f0e91a124a/crates/turborepo-lib/src/run/summary/mod.rs#L770
Since this sort only applies to SinglePackageRunSummary, it made more sense to do it when we care about sorting. If the implementation is the same as in Go, it should apply to dry runs as well as run summary!
I can also revert this part if we aren't confident about that.
There was a problem hiding this comment.
I would say we remove the second sort in normalize over this one as other callers might depend on the list being sorted.
There was a problem hiding this comment.
But then I'd also need another sort somewhere else for monorepos (this one that is removed is only for when converting from monorepo to single package summary).
but point made, I'll keep this one and the duplicate since it's not related to this fix anyway
| dependencies: dependencies | ||
| .into_iter() | ||
| .map(|task_id| task_id.task().to_string()) | ||
| .sorted() |
There was a problem hiding this comment.
This from implementation is used by the into() call here: https://github.com/vercel/turbo/blob/1dd2e93e84e08872f67943804a6600f0e91a124a/crates/turborepo-lib/src/run/summary/task.rs#L211
it only runs for SinglePackageTaskSummary. Since we have moved the sorting to the normalize() step, we shouldn't need it here.
d420e00 to
c471a15
Compare
chris-olszewski
left a comment
There was a problem hiding this comment.
Stamping, but I would love either a unit or integration test. Or just a manual test that shows before/after behavior of this change.
| .cloned() | ||
| .map(SinglePackageTaskSummary::from) | ||
| .collect::<Vec<_>>(); | ||
| tasks.sort_by(|t1, t2| t1.task_id.cmp(&t2.task_id)); |
There was a problem hiding this comment.
I would say we remove the second sort in normalize over this one as other callers might depend on the list being sorted.
Co-authored-by: Chris Olszewski <chris.olszewski@vercel.com>
2d659a7 to
159695b
Compare
No description provided.