Guess extensions on extension not provided #859

kevinkassimo · 2018-09-29T04:23:19Z

Trying to close #857

Guess .ts -> Guess .js -> Give up

Added tests in both tests/ and deno_dir.rs.
(Hope I have not missed something...)

src/deno_dir.rs

ry · 2018-09-30T15:07:06Z

src/http.rs

+      format!("cannot load from '{}'", module_name),
+    ))?;
+  }
+  let body = tokio_util::block_on(response.into_body().concat2())?;


Maybe it doesn't matter but this could be done using only one block_on. It would be nice to have a test showing we get NotFound error when trying to hit 404 url.

So I have somehow combined into a single block_on, but had to explicitly implement Future for a struct holding Concat2 and StatusCode (to implement using enum that either holds body or and error seems more problematic)
(I started to use Rust only very recently, so this might not be a very good solution from the viewpoint of elegance. Would like to learn about better alternatives!)

btw a NotFound test is added

src/deno_dir.rs

ry · 2018-10-01T16:32:40Z

src/http.rs

+      Async::NotReady => Ok(Async::NotReady),
+    }
+  }
+}


This seems not so much related to the current problem - although something like this may be necessary at some point when we address fetch body streaming. I'd rather you not add this here. Either go back to using block_on twice or find some other way of doing this. Have you tried using map() ?

kevinkassimo · 2018-10-01T19:14:18Z

@ry So I have somehow forcefully used map to allow a single block_on (which I have to admit it looks ergonomically very ugly). As I am a Rust user only quite recently, I myself could not found a better solution for the implementation (without messing with manually creating a hyper::Error, which hyper itself is preventing me from doing).

If this still looks pretty bad, I could either shift back to the original 2 block_on implementation, or probably let someone more familiar with Rust to take over this PR and address this very last piece.

Aside: looks like async/await syntax is recently introduced to Rust

src/http.rs

ry · 2018-10-02T05:59:23Z

src/http.rs

+          .concat2()
+          .map(|body| String::from_utf8(body.to_vec()).unwrap())
+          .map_err(|err| DenoError::from(err)),
+      )


I tried to do this more simply myself but couldn't. I think this is correct. Rust is annoyingly painful at times.

Yeah... I found threads complaining similar stuff this morning, and supposedly these issues could be better addressed with the currently experimental async-await syntax

src/deno_dir.rs

CYBAI · 2018-10-02T06:25:27Z

src/deno_dir.rs

+      });
+    };
+    let default_attempt = use_extension("");
+    if let Ok(_) = default_attempt {


If no need to use the value in Ok, would it be better to use with is_some()?

if default_attempt.is_some() { return default_attempt; }

BTW should be

if default_attempt.is_ok() { return default_attempt; }

Thanks!

Ah, yes! Thanks for catching the typo :P

ry

LGTM - Thanks Kevin!

- Improve fetch headers (denoland#853) - Add deno.truncate (denoland#805) - Add copyFile/copyFileSync (denoland#863) - Limit depth of output in console.log for nested objects, and add console.dir (denoland#826) - Guess extensions on extension not provided (denoland#859) - Renames: deno.platform -> deno.platform.os deno.arch -> deno.platform.arch - Upgrade TS to 3.0.3 - Add readDirSync(), readDir() - Add support for TCP servers and clients. (denoland#884) Adds deno.listen(), deno.dial(), deno.Listener and deno.Conn.

- Improve fetch headers (#853) - Add deno.truncate (#805) - Add copyFile/copyFileSync (#863) - Limit depth of output in console.log for nested objects, and add console.dir (#826) - Guess extensions on extension not provided (#859) - Renames: deno.platform -> deno.platform.os deno.arch -> deno.platform.arch - Upgrade TS to 3.0.3 - Add readDirSync(), readDir() - Add support for TCP servers and clients. (#884) Adds deno.listen(), deno.dial(), deno.Listener and deno.Conn.

…and#859)

kevinkassimo changed the title ~~[WIP] Start try extension attempt~~ [WIP] Guess extension on extension not provided Sep 29, 2018

kevinkassimo force-pushed the import/try_ext branch from eaa2195 to ebd46d2 Compare September 29, 2018 05:44

kevinkassimo changed the title ~~[WIP] Guess extension on extension not provided~~ Guess extensions on extension not provided Sep 29, 2018

kevinkassimo force-pushed the import/try_ext branch from 28c792d to f4c9d6e Compare September 29, 2018 06:04

kevinkassimo added 5 commits September 29, 2018 18:13

Start try extension attempt

c6a2413

Add basic rust-side test

bffb412

Add ts side tests

699822b

Ooops

09739fb

Remove duplicate code and simplify

469771e

kevinkassimo force-pushed the import/try_ext branch from f4c9d6e to 469771e Compare September 30, 2018 01:58

ry reviewed Sep 30, 2018

View reviewed changes

src/deno_dir.rs Show resolved Hide resolved

ry reviewed Sep 30, 2018

View reviewed changes

kevinkassimo added 2 commits September 30, 2018 12:30

Add extension test on fetched code length

c4f5adb

Use single block_on and add import failure test

02fbf23

ry reviewed Oct 1, 2018

View reviewed changes

Forcefully use map to address future problems

9da5589

kevinkassimo force-pushed the import/try_ext branch from 742d680 to 70270a0 Compare October 1, 2018 19:28

Drop enum, use DenoError

b9d1ccc

kevinkassimo force-pushed the import/try_ext branch from 70270a0 to b9d1ccc Compare October 1, 2018 19:30

ry reviewed Oct 2, 2018

View reviewed changes

src/http.rs Show resolved Hide resolved

ry reviewed Oct 2, 2018

View reviewed changes

src/deno_dir.rs Show resolved Hide resolved

nit

eb95e04

CYBAI reviewed Oct 2, 2018

View reviewed changes

nit 2

c211eb7

kevinkassimo force-pushed the import/try_ext branch from e553617 to c211eb7 Compare October 2, 2018 06:30

ry approved these changes Oct 2, 2018

View reviewed changes

ry merged commit eba58b7 into denoland:master Oct 2, 2018

ry mentioned this pull request Oct 4, 2018

v0.1.7 #902

Merged

This was referenced Oct 5, 2018

Implement closeRead/closeWrite using TcpStream::shutdown #903

Merged

Handling of urls which don't have file extension #702

Closed

kevinkassimo deleted the import/try_ext branch December 27, 2019 07:52

hardfist pushed a commit to hardfist/deno that referenced this pull request Aug 7, 2024

perf: use Object::with_prototype_and_properties for CallSite (denol…

0a28b7a

…and#859)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Guess extensions on extension not provided #859

Guess extensions on extension not provided #859

kevinkassimo commented Sep 29, 2018 •

edited

Loading

ry Sep 30, 2018

kevinkassimo Sep 30, 2018

ry Oct 1, 2018

kevinkassimo commented Oct 1, 2018 •

edited

Loading

ry Oct 2, 2018

kevinkassimo Oct 2, 2018

CYBAI Oct 2, 2018

kevinkassimo Oct 2, 2018

kevinkassimo Oct 2, 2018

CYBAI Oct 2, 2018

ry left a comment

Guess extensions on extension not provided #859

Guess extensions on extension not provided #859

Conversation

kevinkassimo commented Sep 29, 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 commented Oct 1, 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

ry left a comment

Choose a reason for hiding this comment

kevinkassimo commented Sep 29, 2018 •

edited

Loading

kevinkassimo commented Oct 1, 2018 •

edited

Loading