-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Simplify src fetch logic AND Auto append suffix in cache search #1322
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Just one comment...
src/deno_dir.rs
Outdated
let mut media_type_filename = filename.to_string(); | ||
media_type_filename.push_str(".mime"); | ||
let mt = Path::new(&media_type_filename); | ||
eprintln!("Try downloading {}...", &module_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep it "Downloading"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have actually made some changes to this just now. Now it looks like
Downloading https://deno.land/x/net/file_server.ts... DONE
Compiling https://deno.land/x/net/file_server.ts
Downloading https://deno.land/x/net/http... NOT FOUND
Downloading https://deno.land/x/net/http.ts... DONE
Downloading https://deno.land/x/net/bufio.ts... DONE
Downloading https://deno.land/x/net/textproto.ts... DONE
Downloading https://deno.land/x/net/http_status... NOT FOUND
Downloading https://deno.land/x/net/http_status.ts... DONE
Downloading https://deno.land/x/net/util... NOT FOUND
Downloading https://deno.land/x/net/util.ts... DONE
Compiling https://deno.land/x/net/http.ts
...
HTTP server listening on https://0.0.0.0:4500/
Does this look better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather you keep it as a single line - in the case of 404 print "filename not found" or something. The reason is that I want to refactor this whole bit soon with progress bars - so I prefer if you keep it the same and as simple as possible for future refactors. Ref #1320
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay. Done.
99d3245
to
2c3149d
Compare
src/deno_dir.rs
Outdated
}; | ||
Ok(src) | ||
return Ok(Some(CodeFetchOutput { | ||
module_name: module_name.clone(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clone()
seems unnecessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm module_name.clone()
is unnecessary, but filename.clone()
is still needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I'll make a final push after you complete review... (travis is jammed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? You have created a local filename : String
which shadows filename: &str
in the argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was borrowed: let p = Path::new(&filename);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p
is inactive after its last usage. It should compile when NLL is enabled. Before that, we can only bypass this restriction (annoying). std::mem::drop(p)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a TODO ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err, map_content_type(&p, Some(&content_type))
should also move to another place... Yeah, it depends on you whether to choose this style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@F001 I'll drop it and add a TODO...
Ah yikes. In that case, we have to leave p in its place... NLL comments added nevertheless
7222217
to
0cbf30c
Compare
0cbf30c
to
d307eb0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - this is a nice fix! It's been bugging me for weeks.
- console.assert should not throw error (denoland#1335) - Support more modes in deno.open (denoland#1282, denoland#1336) - Simplify code fetch logic (denoland#1322) - readDir entry mode (denoland#1326) - Use stderr for exceptions (denoland#1303) - console.log formatting improvements (denoland#1327, denoland#1299) - Expose TooLarge error code for buffers (denoland#1298)
Closes #1202
This PR simplifies
get_source_code
logic and perform auto suffix appending attempts when searching local cache.fetch_remote_source
become only fetching remote sourcesfetch_local_source
is added for local/cached sourcesBefore:
After: