-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
7 Ignored Deployments
|
🟢 Turbopack Benchmark CI successful 🟢Thanks |
🟢 CI successful 🟢Thanks |
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
_ = exit_rx => { | ||
tracing::debug!("exiting package changes watcher due to signal"); | ||
}, | ||
_ = process => { |
There was a problem hiding this comment.
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?
&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(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
4183911
to
d576090
Compare
814a01c
to
f039634
Compare
Fix clippy issues
2f49ea9
to
6eb482d
Compare
Description
Added a watcher in
FileWatching
to watch for file changes and map them to packages. Currently you can run this viaturbo 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