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

Partial reload #3109

Merged
merged 15 commits into from
Oct 17, 2019
38 changes: 37 additions & 1 deletion cli/file_fetcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ pub struct SourceFileFetcher {
deps_cache: DiskCache,
progress: Progress,
source_file_cache: SourceFileCache,
cache_blacklist: Vec<String>,
use_disk_cache: bool,
no_remote_fetch: bool,
}
Expand All @@ -79,12 +80,14 @@ impl SourceFileFetcher {
deps_cache: DiskCache,
progress: Progress,
use_disk_cache: bool,
cache_blacklist: Vec<String>,
no_remote_fetch: bool,
) -> std::io::Result<Self> {
let file_fetcher = Self {
deps_cache,
progress,
source_file_cache: SourceFileCache::default(),
cache_blacklist,
use_disk_cache,
no_remote_fetch,
};
Expand Down Expand Up @@ -308,8 +311,10 @@ impl SourceFileFetcher {
return Box::new(futures::future::err(too_many_redirects()));
}

let is_blacklisted =
check_cache_blacklist(module_url, self.cache_blacklist.as_ref());
// First try local cache
if use_disk_cache {
if use_disk_cache && !is_blacklisted {
match self.fetch_cached_remote_source(&module_url) {
Ok(Some(source_file)) => {
return Box::new(futures::future::ok(source_file));
Expand Down Expand Up @@ -533,6 +538,19 @@ fn filter_shebang(bytes: Vec<u8>) -> Vec<u8> {
}
}

fn check_cache_blacklist(url: &Url, black_list: &[String]) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Should the black_list argument be a Vec<Url> instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we never utilize any of the Url properties by elements in the list, so I thought that maybe strings would be better (as in --allow-read whitelist)

but I agree that Vec<Url> would be more clean, and parsing few urls wouldn't harm performance

Copy link
Member

@ry ry Oct 17, 2019

Choose a reason for hiding this comment

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

Yea, I guess it's not a big deal.

I'm ready to land this patch.... but maybe you could add a short description of this feature to the website/manual.md somewhere? I think a sentence or two would do it in the section where --reload is described.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

let mut path_buf = PathBuf::from(url.as_str());
loop {
if black_list.contains(&String::from(path_buf.to_str().unwrap())) {
return true;
}
if !path_buf.pop() {
break;
}
}
false
}

#[derive(Debug, Default)]
/// Header metadata associated with a particular "symbolic" source code file.
/// (the associated source code file might not be cached, while remaining
Expand Down Expand Up @@ -617,6 +635,7 @@ mod tests {
DiskCache::new(&dir_path.to_path_buf().join("deps")),
Progress::new(),
true,
vec![],
false,
)
.expect("setup fail")
Expand All @@ -638,6 +657,23 @@ mod tests {
};
}

#[test]
fn test_cache_blacklist() {
let args = vec![
String::from("http://deno.land/std"),
String::from("http://github.com/example/mod.ts"),
Copy link
Member

Choose a reason for hiding this comment

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

@mhvsa please add test cases with query string (?foo=bar) and fragment (#something)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ay, forgot about that, thank you

];
let url1: Url = "http://deno.land/std/fs/mod.ts".parse().unwrap();
let url2: Url = "http://github.com/example/file.ts".parse().unwrap();
let url3: Url = "http://github.com/example/mod.ts".parse().unwrap();
let result1 = check_cache_blacklist(&url1, &args);
let result2 = check_cache_blacklist(&url2, &args);
let result3 = check_cache_blacklist(&url3, &args);
assert_eq!(result1, true);
assert_eq!(result2, false);
assert_eq!(result3, true);
}

#[test]
fn test_source_code_headers_get_and_save() {
let (_temp_dir, fetcher) = test_setup();
Expand Down
34 changes: 33 additions & 1 deletion cli/flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use log::Level;
use std;
use std::str;
use std::str::FromStr;
use url::Url;

macro_rules! std_url {
($x:expr) => {
Expand Down Expand Up @@ -45,6 +46,7 @@ pub struct DenoFlags {
pub import_map_path: Option<String>,
pub allow_read: bool,
pub read_whitelist: Vec<String>,
pub cache_blacklist: Vec<String>,
pub allow_write: bool,
pub write_whitelist: Vec<String>,
pub allow_net: bool,
Expand Down Expand Up @@ -172,6 +174,10 @@ To get help on the another subcommands (run in this case):
).arg(
Arg::with_name("reload")
.short("r")
.min_values(0)
.takes_value(true)
.use_delimiter(true)
.require_equals(true)
Comment on lines +179 to +180
Copy link
Member

Choose a reason for hiding this comment

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

Are those required? Would be nice to do --reload https://deno.land/std/fs/utils.ts https://deno.land/std/fmt/colors.ts

.long("reload")
.help("Reload source code cache (recompile TypeScript)")
Copy link
Member

Choose a reason for hiding this comment

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

Please add note that blacklist is supported and also add long_help with a few examples.

.global(true),
Expand Down Expand Up @@ -509,6 +515,23 @@ fn resolve_paths(paths: Vec<String>) -> Vec<String> {
out
}

fn resolve_urls(urls: Vec<String>) -> Vec<String> {
let mut out: Vec<String> = vec![];
for urlstr in urls.iter() {
let result = Url::from_str(urlstr);
if result.is_err() {
eprintln!("Bad Url: {}", urlstr);
Copy link
Member

Choose a reason for hiding this comment

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

Error instead of continuing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe in file whitelist also in that case?

continue;
}
let mut full_url = String::from(result.unwrap().as_str());
if full_url.len() > 1 && full_url.ends_with('/') {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this pop needed? Can you add a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for prefix matching, it's the same thing as in files whitelist
it removes everything after '/' from the address, so e.g /foo/bar -> /foo

full_url.pop();
}
out.push(full_url);
}
out
}

/// Parse ArgMatches into internal DenoFlags structure.
/// This method should not make any side effects.
pub fn parse_flags(
Expand All @@ -531,7 +554,16 @@ pub fn parse_flags(
flags.version = true;
}
if matches.is_present("reload") {
flags.reload = true;
if matches.value_of("reload").is_some() {
let cache_bl = matches.values_of("reload").unwrap();
let raw_cache_blacklist: Vec<String> =
cache_bl.map(std::string::ToString::to_string).collect();
flags.cache_blacklist = resolve_urls(raw_cache_blacklist);
debug!("cache blacklist: {:#?}", &flags.cache_blacklist);
flags.reload = false;
Copy link
Member

Choose a reason for hiding this comment

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

Superficial

} else {
flags.reload = true;
}
}
flags.config_path = matches.value_of("config").map(ToOwned::to_owned);
if matches.is_present("v8-options") {
Expand Down
3 changes: 3 additions & 0 deletions cli/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,10 +222,13 @@ impl ThreadSafeState {

let dir = deno_dir::DenoDir::new(custom_root)?;

let cache_bl = flags.cache_blacklist.clone();
Copy link
Member

Choose a reason for hiding this comment

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

Superficial, just do the clone inline


let file_fetcher = SourceFileFetcher::new(
dir.deps_cache.clone(),
progress.clone(),
!flags.reload,
cache_bl,
flags.no_fetch,
)?;

Expand Down