Simplify src fetch logic AND Auto append suffix in cache search #1322

kevinkassimo · 2018-12-12T02:59:14Z

This PR simplifies get_source_code logic and perform auto suffix appending attempts when searching local cache.

fetch_remote_source become only fetching remote sources
fetch_local_source is added for local/cached sources

Before:

# with NO cache
$ deno --allow-net https://deno.land/x/net/file_server.ts
Downloading https://deno.land/x/net/file_server.ts
Compiling https://deno.land/x/net/file_server.ts
Downloading https://deno.land/x/net/http
Downloading https://deno.land/x/net/http.ts
Downloading https://deno.land/x/net/bufio.ts
Downloading https://deno.land/x/net/textproto.ts
Downloading https://deno.land/x/net/http_status
Downloading https://deno.land/x/net/http_status.ts
Downloading https://deno.land/x/net/util
Downloading https://deno.land/x/net/util.ts
Compiling https://deno.land/x/net/http.ts
# ...
HTTP server listening on https://0.0.0.0:4500/

# with cached
$ deno --allow-net https://deno.land/x/net/file_server.ts
Downloading https://deno.land/x/net/http
Downloading https://deno.land/x/net/http_status
Downloading https://deno.land/x/net/util
HTTP server listening on https://0.0.0.0:4500/

After:

# with NO cache
$ deno --allow-net https://deno.land/x/net/file_server.ts
Downloading https://deno.land/x/net/file_server.ts...
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...
Downloading https://deno.land/x/net/bufio.ts...
Downloading https://deno.land/x/net/textproto.ts...
Downloading https://deno.land/x/net/http_status... NOT FOUND
Downloading https://deno.land/x/net/http_status.ts...
Downloading https://deno.land/x/net/util... NOT FOUND
Downloading https://deno.land/x/net/util.ts...
Compiling https://deno.land/x/net/http.ts
# ...
HTTP server listening on https://0.0.0.0:4500/

# with cache
$ deno --allow-net https://deno.land/x/net/file_server.ts
HTTP server listening on https://0.0.0.0:4500/

ry

Great! Just one comment...

ry · 2018-12-12T03:01:54Z

src/deno_dir.rs

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


Please keep it "Downloading"

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?

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

Oh okay. Done.

src/deno_dir.rs

F001 · 2018-12-12T03:43:26Z

src/deno_dir.rs

- };
- Ok(src)
+ return Ok(Some(CodeFetchOutput {
+ module_name: module_name.clone(),


The clone() seems unnecessary?

Hmm module_name.clone() is unnecessary, but filename.clone() is still needed

Also I'll make a final push after you complete review... (travis is jammed)

why? You have created a local filename : String which shadows filename: &str in the argument.

It was borrowed: let p = Path::new(&filename);

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) ?

Add a TODO ?

Err, map_content_type(&p, Some(&content_type)) should also move to another place... Yeah, it depends on you whether to choose this style.

@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

ry

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)

- console.assert should not throw error (#1335) - Support more modes in deno.open (#1282, #1336) - Simplify code fetch logic (#1322) - readDir entry mode (#1326) - Use stderr for exceptions (#1303) - console.log formatting improvements (#1327, #1299) - Expose TooLarge error code for buffers (#1298)

Simplify src fetch logic AND Auto append suffix in cache search

c369d05

ry reviewed Dec 12, 2018

View reviewed changes

kevinkassimo added 2 commits December 11, 2018 19:09

Use emoji

68106f9

Remove fancy logging

2c3149d

kevinkassimo force-pushed the source/fetch_simplify branch from 99d3245 to 2c3149d Compare December 12, 2018 03:17

F001 reviewed Dec 12, 2018

View reviewed changes

src/deno_dir.rs Outdated Show resolved Hide resolved

format! -> array concat

1a962d0

F001 reviewed Dec 12, 2018

View reviewed changes

kevinkassimo force-pushed the source/fetch_simplify branch from 7222217 to 0cbf30c Compare December 12, 2018 04:23

nit

d307eb0

kevinkassimo force-pushed the source/fetch_simplify branch from 0cbf30c to d307eb0 Compare December 12, 2018 04:46

ry approved these changes Dec 12, 2018

View reviewed changes

ry merged commit 65dd0d5 into denoland:master Dec 12, 2018

ry mentioned this pull request Dec 14, 2018

v0.2.3 #1353

Merged

kitsonk mentioned this pull request Dec 17, 2018

Remove DENO_BUILD_PATH and DENO_BUILD_MODE env vars #1356

Closed

kevinkassimo deleted the source/fetch_simplify branch December 27, 2019 07:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify src fetch logic AND Auto append suffix in cache search #1322

Simplify src fetch logic AND Auto append suffix in cache search #1322

kevinkassimo commented Dec 12, 2018 •

edited

Loading

ry left a comment

ry Dec 12, 2018

kevinkassimo Dec 12, 2018 •

edited

Loading

ry Dec 12, 2018 •

edited

Loading

kevinkassimo Dec 12, 2018

F001 Dec 12, 2018

kevinkassimo Dec 12, 2018

kevinkassimo Dec 12, 2018

F001 Dec 12, 2018

kevinkassimo Dec 12, 2018 •

edited

Loading

F001 Dec 12, 2018 •

edited

Loading

ry Dec 12, 2018

F001 Dec 12, 2018

kevinkassimo Dec 12, 2018 •

edited

Loading

ry left a comment

Simplify src fetch logic AND Auto append suffix in cache search #1322

Simplify src fetch logic AND Auto append suffix in cache search #1322

Conversation

kevinkassimo commented Dec 12, 2018 • edited Loading

ry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkassimo Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

ry Dec 12, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkassimo Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

F001 Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkassimo Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

ry left a comment

Choose a reason for hiding this comment

kevinkassimo commented Dec 12, 2018 •

edited

Loading

kevinkassimo Dec 12, 2018 •

edited

Loading

ry Dec 12, 2018 •

edited

Loading

kevinkassimo Dec 12, 2018 •

edited

Loading

F001 Dec 12, 2018 •

edited

Loading

kevinkassimo Dec 12, 2018 •

edited

Loading