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

Guess extensions on extension not provided #859

Merged
merged 11 commits into from
Oct 2, 2018
95 changes: 74 additions & 21 deletions src/deno_dir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,19 +133,41 @@ impl DenoDir {
self: &DenoDir,
module_name: &str,
filename: &str,
) -> DenoResult<String> {
if is_remote(module_name) {
self.fetch_remote_source(module_name, filename)
} else if module_name.starts_with(ASSET_PREFIX) {
) -> DenoResult<CodeFetchOutput> {
if module_name.starts_with(ASSET_PREFIX) {
panic!("Asset resolution should be done in JS, not Rust.");
} else {
assert_eq!(
module_name, filename,
"if a module isn't remote, it should have the same filename"
);
let src = fs::read_to_string(Path::new(filename))?;
Ok(src)
}
let is_module_remote = is_remote(module_name);
let use_extension = |ext| {
let module_name = format!("{}{}", module_name, ext);
let filename = format!("{}{}", filename, ext);
let source_code = if is_module_remote {
self.fetch_remote_source(&module_name, &filename)?
} else {
assert_eq!(
module_name, filename,
"if a module isn't remote, it should have the same filename"
);
fs::read_to_string(Path::new(&filename))?
};
return Ok(CodeFetchOutput {
module_name: module_name.to_string(),
filename: filename.to_string(),
source_code,
maybe_output_code: None,
});
};
let default_attempt = use_extension("");
if let Ok(_) = default_attempt {
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW should be

if default_attempt.is_ok() {
  return default_attempt;
}

Thanks!

Copy link

Choose a reason for hiding this comment

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

Ah, yes! Thanks for catching the typo :P

return default_attempt;
}
debug!("Trying {}.ts...", module_name);
let ts_attempt = use_extension(".ts");
if let Ok(_) = ts_attempt {
return ts_attempt;
}
debug!("Trying {}.js...", module_name);
use_extension(".js")
}

pub fn code_fetch(
Expand All @@ -161,16 +183,7 @@ impl DenoDir {
let (module_name, filename) =
self.resolve_module(module_specifier, containing_file)?;

let result = self
.get_source_code(module_name.as_str(), filename.as_str())
.and_then(|source_code| {
Ok(CodeFetchOutput {
module_name,
filename,
source_code,
maybe_output_code: None,
})
});
let result = self.get_source_code(module_name.as_str(), filename.as_str());
let out = match result {
Err(err) => {
if err.kind() == ErrorKind::NotFound {
Expand Down Expand Up @@ -420,6 +433,46 @@ fn test_code_fetch() {
//println!("code_fetch_output {:?}", code_fetch_output);
}

#[test]
fn test_code_fetch_no_ext() {
let (_temp_dir, deno_dir) = test_setup();

let cwd = std::env::current_dir().unwrap();
let cwd_string = String::from(cwd.to_str().unwrap()) + "/";

// Assuming cwd is the deno repo root.
let module_specifier = "./js/main";
let containing_file = cwd_string.as_str();
let r = deno_dir.code_fetch(module_specifier, containing_file);
assert!(r.is_ok());

// Test .ts extension
// Assuming cwd is the deno repo root.
let module_specifier = "./js/main";
let containing_file = cwd_string.as_str();
let r = deno_dir.code_fetch(module_specifier, containing_file);
assert!(r.is_ok());
let code_fetch_output = r.unwrap();
// could only test .ends_with to avoid include local abs path
assert!(code_fetch_output.module_name.ends_with("/js/main.ts"));
assert!(code_fetch_output.source_code.len() > 10);

// Test .js extension
// Assuming cwd is the deno repo root.
let module_specifier = "./js/mock_builtin";
let containing_file = cwd_string.as_str();
let r = deno_dir.code_fetch(module_specifier, containing_file);
assert!(r.is_ok());
kevinkassimo marked this conversation as resolved.
Show resolved Hide resolved
let code_fetch_output = r.unwrap();
// could only test .ends_with to avoid include local abs path
assert!(
code_fetch_output
.module_name
.ends_with("/js/mock_builtin.js")
);
kevinkassimo marked this conversation as resolved.
Show resolved Hide resolved
assert!(code_fetch_output.source_code.len() > 10);
}

#[test]
fn test_src_file_to_url_1() {
let (_temp_dir, deno_dir) = test_setup();
Expand Down
29 changes: 23 additions & 6 deletions src/http.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
// Copyright 2018 the Deno authors. All rights reserved. MIT license.

use errors::DenoResult;
use errors;
use errors::{DenoError, DenoResult};
use futures;
use futures::future::Either;
use futures::Future;
use tokio_util;

use futures::Future;
use futures::Stream;
use hyper;
use hyper::client::Client;
kevinkassimo marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -31,11 +34,25 @@ pub fn get_client() -> Client<Connector, hyper::Body> {
pub fn fetch_sync_string(module_name: &str) -> DenoResult<String> {
let url = module_name.parse::<Uri>().unwrap();
let client = get_client();
let future = client
let fetch_future = client
.get(url)
.and_then(|response| response.into_body().concat2());
let body = tokio_util::block_on(future)?;
Ok(String::from_utf8(body.to_vec()).unwrap())
.map_err(|err| DenoError::from(err))
.and_then(|response| {
if !response.status().is_success() {
return Either::A(futures::future::err(errors::new(
errors::ErrorKind::NotFound,
"module not found".to_string(),
)));
}
Either::B(
response
.into_body()
.concat2()
.map(|body| String::from_utf8(body.to_vec()).unwrap())
.map_err(|err| DenoError::from(err)),
)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

});
tokio_util::block_on(fetch_future)
}

/* TODO(ry) Re-enabled this test. Disabling to work around bug in #782.
Expand Down
10 changes: 10 additions & 0 deletions tests/015_import_no_ext.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { isTSFile, printHello, phNoExt } from "./subdir/mod3";
console.log(isTSFile);
console.log(printHello);
console.log(phNoExt);

import { isMod4 } from "./subdir/mod4";
console.log(isMod4);

import { printHello as ph } from "http://localhost:4545/tests/subdir/mod2";
console.log(ph);
8 changes: 8 additions & 0 deletions tests/015_import_no_ext.ts.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
Downloading http://localhost:4545/tests/subdir/mod2
Downloading http://localhost:4545/tests/subdir/mod2.ts
Downloading http://localhost:4545/tests/subdir/print_hello.ts
true
[Function: printHello]
[Function: printHello]
true
[Function: printHello]
1 change: 1 addition & 0 deletions tests/error_006_import_ext_failure.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import "./non-existent";
11 changes: 11 additions & 0 deletions tests/error_006_import_ext_failure.ts.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
NotFound: Cannot resolve module "./non-existent" from "[WILDCARD]deno/tests/error_006_import_ext_failure.ts"
at maybeError (deno/js/errors.ts:[WILDCARD])
at maybeThrowError (deno/js/errors.ts:[WILDCARD])
at sendSync (deno/js/dispatch.ts:[WILDCARD])
at Object.codeFetch (deno/js/os.ts:[WILDCARD])
at DenoCompiler.resolveModule (deno/js/compiler.ts:[WILDCARD])
at DenoCompiler._resolveModuleName (deno/js/compiler.ts:[WILDCARD])
at moduleNames.map.name (deno/js/compiler.ts:[WILDCARD])
at Array.map (<anonymous>)
at DenoCompiler.resolveModuleNames (deno/js/compiler.ts:[WILDCARD])
at Object.compilerHost.resolveModuleNames (deno/third_party/node_modules/typescript/lib/typescript.js:[WILDCARD])
1 change: 1 addition & 0 deletions tests/subdir/mod3.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const isTSFile = false;
3 changes: 3 additions & 0 deletions tests/subdir/mod3.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export const isTSFile = true;
export { printHello } from "./print_hello.ts";
export { printHello as phNoExt } from "./print_hello";
1 change: 1 addition & 0 deletions tests/subdir/mod4.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const isMod4 = true;