Partial reload #3109

mhvsa · 2019-10-11T20:07:45Z

bartlomieju

Looking good, a few remarks!

bartlomieju · 2019-10-12T11:13:36Z

cli/file_fetcher.rs

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


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

ay, forgot about that, thank you

bartlomieju · 2019-10-12T11:14:13Z

cli/flags.rs

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


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

bartlomieju · 2019-10-12T11:14:36Z

cli/flags.rs

+        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;


Superficial

bartlomieju · 2019-10-12T11:14:49Z

cli/flags.rs

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


Error instead of continuing

maybe in file whitelist also in that case?

bartlomieju · 2019-10-12T11:15:19Z

cli/flags.rs

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


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

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

bartlomieju

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.

bartlomieju · 2019-10-14T21:00:41Z

cli/flags.rs

+        .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")


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

bartlomieju · 2019-10-14T21:01:23Z

cli/flags.rs

+        .use_delimiter(true)
+        .require_equals(true)


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

bartlomieju · 2019-10-14T21:01:49Z

cli/state.rs

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

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

+    let cache_bl = flags.cache_blacklist.clone();


Superficial, just do the clone inline

…rtial_reload

ry · 2019-10-16T15:16:02Z

cli/flags.rs

+          --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")


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.

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

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

cli/file_fetcher.rs

ry · 2019-10-16T15:20:44Z

cli/file_fetcher.rs

@@ -533,6 +538,26 @@ fn filter_shebang(bytes: Vec<u8>) -> Vec<u8> {
  }
 }

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


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

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

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.

ry · 2019-10-16T15:21:00Z

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

ry · 2019-10-17T14:27:02Z

website/manual.md

+a comma to separate URLs
+
+`--reload=https://deno.land/std/fs/utils.ts,https://deno.land/std/fmt/colors.ts`
+


ry

LGTM - nice work

mhvsa added 3 commits October 9, 2019 21:36

reload flag matches allow-read flag

adb4b4f

implement reload with argument flag

776f1b3

fix check_cache_blacklist fn header

150c769

bartlomieju reviewed Oct 12, 2019

View reviewed changes

mhvsa added 4 commits October 14, 2019 21:40

added query string and fragments support & resolve_url now panics

5487adf

updated --reload help

6c28ac9

fmt

6a9bc63

resolved merging conflicts

08e40ae

bartlomieju reviewed Oct 14, 2019

View reviewed changes

fix long_help & superficial code

1e73e4f

mhvsa closed this Oct 16, 2019

mhvsa reopened this Oct 16, 2019

mhvsa added 2 commits October 16, 2019 16:38

fix long_help & superficial code

fc72a66

Merge branch 'partial_reload' of http://github.com/mhvsa/deno into pa…

cb55b85

…rtial_reload

ry reviewed Oct 16, 2019

View reviewed changes

cli/file_fetcher.rs Outdated Show resolved Hide resolved

ry reviewed Oct 16, 2019

View reviewed changes

ry and others added 5 commits October 16, 2019 13:13

Merge branch 'master' into partial_reload

360714c

cleanup

83ccb74

Merge branch 'master' into partial_reload

5839b11

updated website manual with --reload=<CACHE_BLACKLIST> info

39858bf

format website manual

3358bb6

ry reviewed Oct 17, 2019

View reviewed changes

website/manual.md

a comma to separate URLs

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

Copy link

Member

ry Oct 17, 2019

Choose a reason for hiding this comment

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

Thanks!

ry approved these changes Oct 17, 2019

View reviewed changes

ry merged commit 75ec942 into denoland:master Oct 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial reload #3109

Partial reload #3109

mhvsa commented Oct 11, 2019

bartlomieju left a comment

bartlomieju Oct 12, 2019

mhvsa Oct 12, 2019

bartlomieju Oct 12, 2019

bartlomieju Oct 12, 2019

bartlomieju Oct 12, 2019

mhvsa Oct 12, 2019

bartlomieju Oct 12, 2019

mhvsa Oct 12, 2019

bartlomieju left a comment

bartlomieju Oct 14, 2019

bartlomieju Oct 14, 2019

bartlomieju Oct 14, 2019

ry Oct 16, 2019

bartlomieju Oct 16, 2019 •

edited

Loading

ry Oct 16, 2019

ry Oct 16, 2019

mhvsa Oct 16, 2019

ry Oct 17, 2019 •

edited

Loading

mhvsa Oct 17, 2019

ry commented Oct 16, 2019

ry Oct 17, 2019

ry left a comment

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

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

		let cache_bl = flags.cache_blacklist.clone();

		a comma to separate URLs

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

Partial reload #3109

Partial reload #3109

Conversation

mhvsa commented Oct 11, 2019

bartlomieju left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju Oct 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry Oct 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry commented Oct 16, 2019

Choose a reason for hiding this comment

ry left a comment

Choose a reason for hiding this comment

bartlomieju Oct 16, 2019 •

edited

Loading

ry Oct 17, 2019 •

edited

Loading