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

chore(visitor): break up visitor to multiple modules #9306

Merged
merged 13 commits into from
Oct 23, 2024

Conversation

chris-olszewski
Copy link
Member

Description

This is a prefactor to break up visitor.rs into multiple files. This is primarily just moving code around, but there are a few more in depth changes:

  • 2873556 changes TaskWarning to only be constructed if there are missing envs, removing the need to check later on
  • 88e98c5 changes so we no longer call which for every task to find the package manager binary. We instead call this on construction, but will only handle the error case on actual command creation.

I highly recommend reviewing each commit on it's own. All commits that move code do only that so hopefully that makes it easier to review.

Testing Instructions

Integration tests and some basic manual testing.

Copy link

vercel bot commented Oct 22, 2024

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

Name Status Preview Comments Updated (UTC)
examples-nonmonorepo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 23, 2024 0:36am
8 Skipped Deployments
Name Status Preview Comments Updated (UTC)
examples-basic-web ⬜️ Ignored (Inspect) Oct 23, 2024 0:36am
examples-designsystem-docs ⬜️ Ignored (Inspect) Oct 23, 2024 0:36am
examples-gatsby-web ⬜️ Ignored (Inspect) Oct 23, 2024 0:36am
examples-kitchensink-blog ⬜️ Ignored (Inspect) Oct 23, 2024 0:36am
examples-native-web ⬜️ Ignored (Inspect) Oct 23, 2024 0:36am
examples-svelte-web ⬜️ Ignored (Inspect) Oct 23, 2024 0:36am
examples-tailwind-web ⬜️ Ignored (Inspect) Oct 23, 2024 0:36am
examples-vite-web ⬜️ Ignored (Inspect) Oct 23, 2024 0:36am

@chris-olszewski chris-olszewski marked this pull request as ready for review October 22, 2024 14:27
@chris-olszewski chris-olszewski requested a review from a team as a code owner October 22, 2024 14:27
Copy link
Contributor

@dimitropoulos dimitropoulos left a comment

Choose a reason for hiding this comment

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

I know you just moved it: but wow, visit's a chunky one! don't mean that in a bad way, it's got a lot to do, but just interesting

@chris-olszewski chris-olszewski enabled auto-merge (squash) October 23, 2024 12:35
@chris-olszewski chris-olszewski merged commit cff7529 into main Oct 23, 2024
40 checks passed
@chris-olszewski chris-olszewski deleted the olszewski/feat_injectable_tasks branch October 23, 2024 14:14
chris-olszewski added a commit that referenced this pull request Oct 30, 2024
### Description

In #9306 I somehow dropped the
`cmd.open_stdin()` call so we never started tasks hooked up to stdin.

I verified I copied the closing/forwarding stdin logic in my refactor
[as seen
here](https://github.com/vercel/turborepo/blob/main/crates/turborepo-lib/src/task_graph/visitor/exec.rs#L376),
we were just never opening it in the first place.

### Testing Instructions

`turbo_dev dev` should now let you interact with `persistent` tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants