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

Merged
merged 9 commits into from
Dec 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
90 changes: 68 additions & 22 deletions cli/flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ pub struct Flags {
pub unsafely_ignore_certificate_errors: Option<Vec<String>>,
pub v8_flags: Vec<String>,
pub version: bool,
pub watch: bool,
pub watch: Option<Vec<PathBuf>>,
}

fn join_paths(allowlist: &[PathBuf], d: &str) -> String {
Expand Down 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,14 +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")
.help("UNSTABLE: Watch for file changes and restart process automatically")
.long_help(
.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 @@ -1743,7 +1758,7 @@ fn bundle_parse(flags: &mut Flags, matches: &clap::ArgMatches) {
None
};

flags.watch = matches.is_present("watch");
watch_arg_parse(flags, matches, false);

flags.subcommand = DenoSubcommand::Bundle(BundleFlags {
source_file,
Expand Down Expand Up @@ -1874,7 +1889,7 @@ fn eval_parse(flags: &mut Flags, matches: &clap::ArgMatches) {

fn fmt_parse(flags: &mut Flags, matches: &clap::ArgMatches) {
config_arg_parse(flags, matches);
flags.watch = matches.is_present("watch");
watch_arg_parse(flags, matches, false);
let files = match matches.values_of("files") {
Some(f) => f.map(PathBuf::from).collect(),
None => vec![],
Expand Down Expand Up @@ -1996,7 +2011,7 @@ fn lsp_parse(flags: &mut Flags, _matches: &clap::ArgMatches) {

fn lint_parse(flags: &mut Flags, matches: &clap::ArgMatches) {
config_arg_parse(flags, matches);
flags.watch = matches.is_present("watch");
watch_arg_parse(flags, matches, false);
let files = match matches.values_of("files") {
Some(f) => f.map(PathBuf::from).collect(),
None => vec![],
Expand Down Expand Up @@ -2061,7 +2076,7 @@ fn run_parse(flags: &mut Flags, matches: &clap::ArgMatches) {
flags.argv.push(v);
}

flags.watch = matches.is_present("watch");
watch_arg_parse(flags, matches, true);
flags.subcommand = DenoSubcommand::Run(RunFlags { script });
}

Expand Down Expand Up @@ -2135,7 +2150,7 @@ fn test_parse(flags: &mut Flags, matches: &clap::ArgMatches) {
};

flags.coverage_dir = matches.value_of("coverage").map(String::from);
flags.watch = matches.is_present("watch");
watch_arg_parse(flags, matches, false);
flags.subcommand = DenoSubcommand::Test(TestFlags {
no_run,
doc,
Expand Down Expand Up @@ -2409,6 +2424,20 @@ fn inspect_arg_validate(val: String) -> Result<(), String> {
}
}

fn watch_arg_parse(
flags: &mut Flags,
matches: &clap::ArgMatches,
allow_extra: bool,
) {
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.

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.

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

// TODO(ry) move this to utility module and add test.
/// Strips fragment part of URL. Panics on bad URL.
pub fn resolve_urls(urls: Vec<String>) -> Vec<String> {
Expand Down Expand Up @@ -2513,7 +2542,24 @@ mod tests {
subcommand: DenoSubcommand::Run(RunFlags {
script: "script.ts".to_string(),
}),
watch: true,
watch: Some(vec![]),
..Flags::default()
}
);
}

#[test]
fn run_watch_with_external() {
let r =
flags_from_vec(svec!["deno", "run", "--watch=file1,file2", "script.ts"]);
let flags = r.unwrap();
assert_eq!(
flags,
Flags {
subcommand: DenoSubcommand::Run(RunFlags {
script: "script.ts".to_string(),
}),
watch: Some(vec![PathBuf::from("file1"), PathBuf::from("file2")]),
..Flags::default()
}
);
Expand Down Expand Up @@ -2746,7 +2792,7 @@ mod tests {
single_quote: None,
prose_wrap: None,
}),
watch: true,
watch: Some(vec![]),
..Flags::default()
}
);
Expand All @@ -2773,7 +2819,7 @@ mod tests {
single_quote: None,
prose_wrap: None,
}),
watch: true,
watch: Some(vec![]),
..Flags::default()
}
);
Expand Down Expand Up @@ -2821,7 +2867,7 @@ mod tests {
prose_wrap: None,
}),
config_path: Some("deno.jsonc".to_string()),
watch: true,
watch: Some(vec![]),
..Flags::default()
}
);
Expand Down Expand Up @@ -3543,7 +3589,7 @@ mod tests {
source_file: "source.ts".to_string(),
out_file: None,
}),
watch: true,
watch: Some(vec![]),
..Flags::default()
}
)
Expand Down Expand Up @@ -4280,7 +4326,7 @@ mod tests {
ignore: vec![],
concurrent_jobs: NonZeroUsize::new(1).unwrap(),
}),
watch: false,
watch: None,
..Flags::default()
}
);
Expand All @@ -4303,7 +4349,7 @@ mod tests {
ignore: vec![],
concurrent_jobs: NonZeroUsize::new(1).unwrap(),
}),
watch: true,
watch: Some(vec![]),
..Flags::default()
}
);
Expand Down
18 changes: 13 additions & 5 deletions cli/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,8 @@ async fn lint_command(
None
};

tools::lint::lint(maybe_lint_config, lint_flags, flags.watch).await?;
tools::lint::lint(maybe_lint_config, lint_flags, flags.watch.is_some())
.await?;
Ok(0)
}

Expand Down Expand Up @@ -894,7 +895,7 @@ async fn bundle_command(
}
};

if flags.watch {
if flags.watch.is_some() {
file_watcher::watch_func(resolver, operation, "Bundle").await?;
} else {
let module_graph =
Expand Down Expand Up @@ -943,7 +944,8 @@ async fn format_command(
return Ok(0);
}

tools::fmt::format(fmt_flags, flags.watch, maybe_fmt_config).await?;
tools::fmt::format(fmt_flags, flags.watch.is_some(), maybe_fmt_config)
.await?;
Ok(0)
}

Expand Down Expand Up @@ -1011,6 +1013,7 @@ async fn run_with_watch(flags: Flags, script: String) -> Result<i32, AnyError> {
let script1 = script.clone();
let script2 = script.clone();
let flags = flags.clone();
let watch_flag = flags.watch.clone();
async move {
let main_module = resolve_url_or_path(&script1)?;
let ps = ProcState::build(flags).await?;
Expand Down Expand Up @@ -1067,6 +1070,11 @@ async fn run_with_watch(flags: Flags, script: String) -> Result<i32, AnyError> {
})
.collect();

// Add the extra files listed in the watch flag
if let Some(watch_paths) = watch_flag {
paths_to_watch.extend(watch_paths);
}

if let Some(import_map) = ps.flags.import_map_path.as_ref() {
paths_to_watch
.push(fs_util::resolve_from_cwd(std::path::Path::new(import_map))?);
Expand Down Expand Up @@ -1180,7 +1188,7 @@ async fn run_command(
return run_from_stdin(flags).await;
}

if flags.watch {
if flags.watch.is_some() {
return run_with_watch(flags, run_flags.script).await;
}

Expand Down Expand Up @@ -1294,7 +1302,7 @@ async fn test_command(
);
}

if flags.watch {
if flags.watch.is_some() {
tools::test::run_tests_with_watch(
flags,
test_flags.include,
Expand Down
41 changes: 41 additions & 0 deletions cli/tests/integration/watcher_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -575,6 +575,47 @@ fn run_watch() {
check_alive_then_kill(child);
}

#[test]
fn run_watch_external_watch_files() {
let t = TempDir::new().unwrap();
let file_to_watch = t.path().join("file_to_watch.js");
write(&file_to_watch, "console.log('Hello world');").unwrap();

let external_file_to_watch = t.path().join("external_file_to_watch.txt");
write(&external_file_to_watch, "Hello world").unwrap();

let mut watch_arg = "--watch=".to_owned();
let external_file_to_watch_str = external_file_to_watch
.clone()
.into_os_string()
.into_string()
.unwrap();
watch_arg.push_str(&external_file_to_watch_str);

let mut child = util::deno_cmd()
.current_dir(util::testdata_path())
.arg("run")
.arg(watch_arg)
.arg("--unstable")
.arg(&file_to_watch)
.env("NO_COLOR", "1")
.stdout(std::process::Stdio::piped())
.stderr(std::process::Stdio::piped())
.spawn()
.unwrap();
let (mut stdout_lines, mut stderr_lines) = child_lines(&mut child);

assert_contains!(stdout_lines.next().unwrap(), "Hello world");
wait_for("Process finished", &mut stderr_lines);

// Change content of the external file
write(&external_file_to_watch, "Hello world2").unwrap();

assert_contains!(stderr_lines.next().unwrap(), "Restarting");
wait_for("Process finished", &mut stderr_lines);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to see what happens with the test if watching doesn't work correctly, it seems like the test will hang forever in this case. Not sure if this is an issue.

check_alive_then_kill(child);
}

#[test]
fn run_watch_load_unload_events() {
let t = TempDir::new().unwrap();
Expand Down
2 changes: 1 addition & 1 deletion cli/tools/standalone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,6 @@ pub fn compile_to_runtime_flags(
unstable: flags.unstable,
v8_flags: flags.v8_flags,
version: false,
watch: false,
watch: None,
})
}