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

Conversation

@mhvsa
Copy link
Contributor

mhvsa commented Oct 11, 2019

fixes #2872

Copy link
Contributor

bartlomieju left a comment

Looking good, a few remarks!

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

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Oct 12, 2019

Contributor

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

This comment has been minimized.

Copy link
@mhvsa

mhvsa Oct 12, 2019

Author Contributor

ay, forgot about that, thank you

.min_values(0)
.takes_value(true)
.use_delimiter(true)
.require_equals(true)
.long("reload")
.help("Reload source code cache (recompile TypeScript)")

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Oct 12, 2019

Contributor

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

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;

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Oct 12, 2019

Contributor

Superficial

for urlstr in urls.iter() {
let result = Url::from_str(urlstr);
if result.is_err() {
eprintln!("Bad Url: {}", urlstr);

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Oct 12, 2019

Contributor

Error instead of continuing

This comment has been minimized.

Copy link
@mhvsa

mhvsa Oct 12, 2019

Author Contributor

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('/') {

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Oct 12, 2019

Contributor

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

This comment has been minimized.

Copy link
@mhvsa

mhvsa Oct 12, 2019

Author Contributor

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

Copy link
Contributor

bartlomieju left a comment

Looks good! It just appeared to me that it'd be nice to support regexes as well, but that can be done in separate PR.

.long_help("Reload source code cache (recompile TypeScript). Supports blacklist\
--reload // Reload everything\
--reload=https://deno.land/std // Reload everything from the standard module\
--reload=https://deno.land/std/fs/utils.ts,https://deno.land/std/fmt/colors.ts // Reload only fs/utils and fmt/colors modules")

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Oct 14, 2019

Contributor

s/only fs/utils and fmt/colors modules/only fs/utils.ts and fmt/colors.ts modules

.use_delimiter(true)
.require_equals(true)
Comment on lines +180 to +181

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Oct 14, 2019

Contributor

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

@@ -221,10 +221,13 @@ impl ThreadSafeState {

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

let cache_bl = flags.cache_blacklist.clone();

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Oct 14, 2019

Contributor

Superficial, just do the clone inline

@mhvsa mhvsa closed this Oct 16, 2019
@mhvsa mhvsa reopened this Oct 16, 2019
--reload=https://deno.land/std
Reload everything from the standard module
--reload=https://deno.land/std/fs/utils.ts,https://deno.land/std/fmt/colors.ts
Reloads only fs/utils and fmt/colors modules")

This comment has been minimized.

Copy link
@ry

ry Oct 16, 2019

Collaborator

Shouldn't this be a "whitelist" rather than a "blacklist" ?

Also I don't think it's useful to say "Supports blacklist" or "Supports whitelist" - the examples you give are sufficient.

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Oct 16, 2019

Contributor

That's actually a cache blacklist - urls passed to --reload will be forced to redownloaded and recompile.

This comment has been minimized.

Copy link
@ry

ry Oct 16, 2019

Collaborator

Ah right. I think "blacklist" makes sense elsewhere in the code, but not here in the user facing docs.

cli/file_fetcher.rs Outdated Show resolved Hide resolved
@@ -533,6 +538,26 @@ fn filter_shebang(bytes: Vec<u8>) -> Vec<u8> {
}
}

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

This comment has been minimized.

Copy link
@ry

ry Oct 16, 2019

Collaborator

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

This comment has been minimized.

Copy link
@mhvsa

mhvsa Oct 16, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@ry

ry Oct 17, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@mhvsa

mhvsa Oct 17, 2019

Author Contributor

Sure

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Oct 16, 2019

Nice patch - thanks! Just a few comments...

ry and others added 5 commits Oct 16, 2019
a comma to separate URLs

`--reload=https://deno.land/std/fs/utils.ts,https://deno.land/std/fmt/colors.ts`

This comment has been minimized.

Copy link
@ry

ry Oct 17, 2019

Collaborator

Thanks!

@ry
ry approved these changes Oct 17, 2019
Copy link
Collaborator

ry left a comment

LGTM - nice work

@ry ry merged commit 75ec942 into denoland:master Oct 17, 2019
10 checks passed
10 checks passed
test macOS-10.14
Details
test_std macOS-10.14
Details
test windows-2016
Details
test_std windows-2016
Details
test ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
test_std ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.