-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
refactor: add WatcherInterface helper struct #20927
Conversation
cli/util/file_watcher.rs
Outdated
@@ -109,26 +108,85 @@ fn create_print_after_restart_fn(clear_screen: bool) -> impl Fn() { | |||
} | |||
} | |||
|
|||
// TODO(bartlomieju): this is a poor name; change it | |||
/// And interface to interact with Deno's CLI file watcher. | |||
pub struct WatcherInterface { |
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.
@dsherret any suggestions for the name of this struct?
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.
ChangeNotifierInterface ?
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.
WatcherChannel
maybe (or WatcherCommunicator
). We should probably avoid names that end in Interface
.
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 like WatcherChannel
. It's bidirectional communication between inside and outside the watched operation
This reverts commit 4ff0cc6.
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.
LGTM, but I think us exposing all these channels in higher level code is not ideal. I believe we could make the low level channels not exposed to other parts of the code.
cli/util/file_watcher.rs
Outdated
@@ -109,26 +108,85 @@ fn create_print_after_restart_fn(clear_screen: bool) -> impl Fn() { | |||
} | |||
} | |||
|
|||
// TODO(bartlomieju): this is a poor name; change it | |||
/// And interface to interact with Deno's CLI file watcher. | |||
pub struct WatcherInterface { |
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.
WatcherChannel
maybe (or WatcherCommunicator
). We should probably avoid names that end in Interface
.
tokio::sync::mpsc::unbounded_channel(); | ||
let (restart_tx, mut restart_rx) = tokio::sync::mpsc::unbounded_channel(); | ||
let (changed_paths_tx, changed_paths_rx) = tokio::sync::broadcast::channel(4); |
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.
It would be nice if we could bury all these channels into WatcherInterface
(then rename WatcherInterface
to WatcherChannel
maybe).
Prerequisite for #20876