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.

Sign up for GitHub

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(watch): support watching external files #13087

Merged
merged 9 commits into from
Dec 15, 2021
Merged

feat(watch): support watching external files #13087

Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ae4f843
feat(watch): support watching external files
jespertheend Dec 14, 2021
e58cf42
test(watch) add external watch files test
jespertheend Dec 14, 2021
0fc1d44
lint
jespertheend Dec 14, 2021
b026f46
Change —help text
jespertheend Dec 15, 2021
316a5ea
Lint
jespertheend Dec 15, 2021
5146e77
Only take values in `--watch` for `run` command
jespertheend Dec 15, 2021
7b9d622
Cleanup watch arg parsing
jespertheend Dec 15, 2021
c1d6655
test(watch): add run_watch_with_external test
jespertheend Dec 15, 2021
f1d0009
Format
jespertheend Dec 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 30 additions & 25 deletions cli/flags.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,7 +549,7 @@ fn bundle_subcommand<'a, 'b>() -> App<'a, 'b> {
.required(true),
)
.arg(Arg::with_name("out_file").takes_value(true).required(false))
.arg(watch_arg())
.arg(watch_arg(false))
.about("Bundle module and dependencies into single file")
.long_about(
"Output a single JavaScript file with all dependencies.
Expand Down Expand Up @@ -882,7 +882,7 @@ Ignore formatting a file by adding an ignore comment at the top of the file:
.multiple(true)
.required(false),
)
.arg(watch_arg())
.arg(watch_arg(false))
.arg(
Arg::with_name("options-use-tabs")
.long("options-use-tabs")
Expand Down Expand Up @@ -1158,7 +1158,7 @@ Ignore linting a file by adding an ignore comment at the top of the file:
.multiple(true)
.required(false),
)
.arg(watch_arg())
.arg(watch_arg(false))
}

fn repl_subcommand<'a, 'b>() -> App<'a, 'b> {
Expand All @@ -1177,7 +1177,7 @@ fn repl_subcommand<'a, 'b>() -> App<'a, 'b> {
fn run_subcommand<'a, 'b>() -> App<'a, 'b> {
runtime_args(SubCommand::with_name("run"), true, true)
.arg(
watch_arg()
watch_arg(true)
.conflicts_with("inspect")
.conflicts_with("inspect-brk"),
)
Expand Down Expand Up @@ -1305,7 +1305,7 @@ fn test_subcommand<'a, 'b>() -> App<'a, 'b> {
.multiple(true),
)
.arg(
watch_arg()
watch_arg(false)
.conflicts_with("no-run")
.conflicts_with("coverage"),
)
Expand Down Expand Up @@ -1640,20 +1640,29 @@ fn compat_arg<'a, 'b>() -> Arg<'a, 'b> {
.help("Node compatibility mode. Currently only enables built-in node modules like 'fs' and globals like 'process'.")
}

fn watch_arg<'a, 'b>() -> Arg<'a, 'b> {
Arg::with_name("watch")
fn watch_arg<'a, 'b>(takes_files: bool) -> Arg<'a, 'b> {
let arg = Arg::with_name("watch")
.long("watch")
.value_name("FILES")
.min_values(0)
.takes_value(true)
.use_delimiter(true)
.require_equals(true)
.help("UNSTABLE: Watch for file changes and restart process automatically")
.long_help(
"UNSTABLE: Watch for file changes and restart process automatically.
.help("UNSTABLE: Watch for file changes and restart process automatically");

if takes_files {
arg
.value_name("FILES")
.min_values(0)
.takes_value(true)
.use_delimiter(true)
.require_equals(true)
.long_help(
"UNSTABLE: Watch for file changes and restart process automatically.
Local files from entry point module graph are watched by default.
Additional paths might be watched by passing them as arguments to this flag.",
)
} else {
arg.long_help(
"UNSTABLE: Watch for file changes and restart process automatically.
Only local files from entry point module graph are watched.",
)
}
}

fn no_check_arg<'a, 'b>() -> Arg<'a, 'b> {
Expand Down Expand Up @@ -2420,17 +2429,13 @@ fn watch_arg_parse(
matches: &clap::ArgMatches,
allow_extra: bool,
) {
flags.watch = match matches.values_of("watch") {
Some(f) => {
let extra_files = f.map(PathBuf::from).collect();
if allow_extra {
Some(extra_files)
} else {
Some(vec![])
}
if allow_extra {
if let Some(f) = matches.values_of("watch") {
flags.watch = Some(f.map(PathBuf::from).collect());
Comment on lines +2433 to +2434
Copy link
Member

Choose a reason for hiding this comment

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

Will this still work with deno run --watch? Is there a unit test that covers this case?

Copy link
Member

Choose a reason for hiding this comment

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

Actually I see no unit test that covers deno run --watch=file1,file2, could you add it @jespertheend?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deno/cli/tests/integration/watcher_tests.rs

Line 734 in 6c324ac

fn test_watch() {
should cover deno run --watch. But now that I think about it, it seems like this shouldn't work. I'll investigate.
I'll also add the extra test 👍

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 this works because matches.values_of returns an empty vec when used as --watch without any files.
I've added the extra unit test.

}
None => None,
};
} else if matches.is_present("watch") {
flags.watch = Some(vec![]);
}
}

// TODO(ry) move this to utility module and add test.
Expand Down