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

feat(turborepo): Package Change Watching #7570

Merged
merged 17 commits into from Apr 4, 2024

Conversation

NicholasLYang
Copy link
Contributor

@NicholasLYang NicholasLYang commented Mar 1, 2024

Description

Added a watcher in FileWatching to watch for file changes and map them to packages. Currently you can run this via turbo daemon watch but eventually this will be used for watch mode.

Testing Instructions

Run the daemon and use turbo daemon watch to see the changed package events as you change files.

Closes TURBO-2499

Copy link

vercel bot commented Mar 1, 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 Apr 4, 2024 5:01pm
examples-svelte-web 🔄 Building (Inspect) Visit Preview 💬 Add feedback Apr 4, 2024 5:01pm
rust-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 4, 2024 5:01pm
turbo-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 4, 2024 5:01pm
7 Ignored Deployments
Name Status Preview Comments Updated (UTC)
examples-basic-web ⬜️ Ignored (Inspect) Visit Preview Apr 4, 2024 5:01pm
examples-designsystem-docs ⬜️ Ignored (Inspect) Visit Preview Apr 4, 2024 5:01pm
examples-gatsby-web ⬜️ Ignored (Inspect) Visit Preview Apr 4, 2024 5:01pm
examples-kitchensink-blog ⬜️ Ignored (Inspect) Visit Preview Apr 4, 2024 5:01pm
examples-native-web ⬜️ Ignored (Inspect) Visit Preview Apr 4, 2024 5:01pm
examples-tailwind-web ⬜️ Ignored (Inspect) Visit Preview Apr 4, 2024 5:01pm
examples-vite-web ⬜️ Ignored (Inspect) Visit Preview Apr 4, 2024 5:01pm

Copy link
Contributor

github-actions bot commented Mar 1, 2024

🟢 Turbopack Benchmark CI successful 🟢

Thanks

Copy link
Contributor

github-actions bot commented Mar 1, 2024

🟢 CI successful 🟢

Thanks

Copy link
Contributor

@gsoltis gsoltis left a comment

Choose a reason for hiding this comment

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

This looks like a pretty good start. I have a question about ordering with respect to stream setup and filewatching startup, but once that's resolved I think we're in good shape.

changed_files,
changed_pkgs
);
for pkg in changed_pkgs {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're going to have to decide where in the system debouncing should live. I don't know what the right answer is yet, but some component is going to need to buffer changes and wait for a timeout before kicking off another run.

Not saying it should definitely be here, but worth noting that a consumer of this stream will need to debounce.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think in the client is maybe better since it keeps separation of concerns. Although perhaps if there's performance issues in the watcher, we could consider adding it there.

crates/turborepo-filewatch/src/package_changes_watcher.rs Outdated Show resolved Hide resolved
_ = exit_rx => {
tracing::debug!("exiting package changes watcher due to signal");
},
_ = process => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to above, if we've bailed on process, is there a reason we shouldn't loop around and try again?

crates/turborepo-lib/src/daemon/proto/turbod.proto Outdated Show resolved Hide resolved
&self,
_request: tonic::Request<proto::PackageChangesRequest>,
) -> Result<tonic::Response<Self::PackageChangesStream>, tonic::Status> {
let mut package_changes_rx = self.file_watching.package_changes_watcher.package_changes();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment about ordering here? How does the client know when changes will started to be accounted for? If the stream is open, and then the client makes a change, are we guaranteed to see a change event? Or is it possible that filewatching is still starting up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first event we send is a rediscover event so in theory it shouldn't matter. Even if the client changes something, the first thing it's going to do after connecting to the daemon is build all of the packages

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm misunderstanding, but it doesn't look like that's what it's doing. The watcher task sends a rediscover message first, but there's no guarantee that a client will get that message, right?

Imagine we start the daemon, send the rediscover message, then make some changes, which triggers more package change notifications. Then a client connects and wants to watch package changes. Is there a mechanism for that client to get the rediscover message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah. I'll send a rediscover as the first message when we start up the stream.

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.

None yet

2 participants