From ea6c9f2f365698e8120177bb7a1344e83f859180 Mon Sep 17 00:00:00 2001 From: Ryan Dahl Date: Thu, 3 Jan 2019 22:11:01 -0500 Subject: [PATCH] Revert "use byte array instead of string for code fetch (#1307)" (#1455) This reverts commit e976b3e0414dc768624b77e431ee7f55b03b76a4. There is nothing technically wrong with this commit, but it's adding complexity to a big refactor (native ES modules #975). Since it's not necessary and simply a philosophical preference, I will revert for now and try to bring it back later. --- js/compiler.ts | 1 - js/os.ts | 11 ++-- src/deno_dir.rs | 127 +++++++++++++++++++++++------------------------ src/http_util.rs | 8 +-- src/js_errors.rs | 16 +++--- src/msg.fbs | 14 +++--- src/ops.rs | 8 +-- 7 files changed, 88 insertions(+), 97 deletions(-) diff --git a/js/compiler.ts b/js/compiler.ts index f8899135b500e5..9656bf446fb846 100644 --- a/js/compiler.ts +++ b/js/compiler.ts @@ -233,7 +233,6 @@ export class Compiler // We query Rust with a CodeFetch message. It will load the sourceCode, // and if there is any outputCode cached, will return that as well. const fetchResponse = this._os.codeFetch(moduleSpecifier, containingFile); - assert(fetchResponse != null, "fetchResponse is null"); moduleId = fetchResponse.moduleName; fileName = fetchResponse.filename; mediaType = fetchResponse.mediaType; diff --git a/js/os.ts b/js/os.ts index 7fc37e46a4911e..e56aab574375f0 100644 --- a/js/os.ts +++ b/js/os.ts @@ -4,7 +4,6 @@ import { assert } from "./util"; import * as util from "./util"; import * as flatbuffers from "./flatbuffers"; import { sendSync } from "./dispatch"; -import { TextDecoder } from "./text_encoding"; interface CodeInfo { moduleName: string | undefined; @@ -46,17 +45,13 @@ export function codeFetch(specifier: string, referrer: string): CodeInfo { assert(baseRes!.inner(codeFetchRes) != null); // flatbuffers returns `null` for an empty value, this does not fit well with // idiomatic TypeScript under strict null checks, so converting to `undefined` - const sourceCode = codeFetchRes.sourceCodeArray() || undefined; - const outputCode = codeFetchRes.outputCodeArray() || undefined; - const sourceMap = codeFetchRes.sourceMapArray() || undefined; - const decoder = new TextDecoder(); return { moduleName: codeFetchRes.moduleName() || undefined, filename: codeFetchRes.filename() || undefined, mediaType: codeFetchRes.mediaType(), - sourceCode: sourceCode && decoder.decode(sourceCode), - outputCode: outputCode && decoder.decode(outputCode), - sourceMap: sourceMap && decoder.decode(sourceMap) + sourceCode: codeFetchRes.sourceCode() || undefined, + outputCode: codeFetchRes.outputCode() || undefined, + sourceMap: codeFetchRes.sourceMap() || undefined }; } diff --git a/src/deno_dir.rs b/src/deno_dir.rs index 3b3a2c3d342cf1..d3d67d19535772 100644 --- a/src/deno_dir.rs +++ b/src/deno_dir.rs @@ -11,6 +11,7 @@ use msg; use ring; use std; +use std::fmt::Write; use std::fs; use std::path::Path; use std::path::PathBuf; @@ -23,13 +24,13 @@ pub struct CodeFetchOutput { pub module_name: String, pub filename: String, pub media_type: msg::MediaType, - pub source_code: Vec, - pub maybe_output_code: Option>, - pub maybe_source_map: Option>, + pub source_code: String, + pub maybe_output_code: Option, + pub maybe_source_map: Option, } impl CodeFetchOutput { - pub fn js_source<'a>(&'a self) -> &'a Vec { + pub fn js_source<'a>(&'a self) -> &'a String { match self.maybe_output_code { None => &self.source_code, Some(ref output_code) => output_code, @@ -108,7 +109,7 @@ impl DenoDir { pub fn cache_path( self: &Self, filename: &str, - source_code: &[u8], + source_code: &str, ) -> (PathBuf, PathBuf) { let cache_key = source_code_hash(filename, source_code); ( @@ -120,25 +121,25 @@ impl DenoDir { fn load_cache( self: &Self, filename: &str, - source_code: &[u8], - ) -> Result<(Vec, Vec), std::io::Error> { + source_code: &str, + ) -> Result<(String, String), std::io::Error> { let (output_code, source_map) = self.cache_path(filename, source_code); debug!( "load_cache code: {} map: {}", output_code.display(), source_map.display() ); - let read_output_code = fs::read(&output_code)?; - let read_source_map = fs::read(&source_map)?; + let read_output_code = fs::read_to_string(&output_code)?; + let read_source_map = fs::read_to_string(&source_map)?; Ok((read_output_code, read_source_map)) } pub fn code_cache( self: &Self, filename: &str, - source_code: &[u8], - output_code: &[u8], - source_map: &[u8], + source_code: &str, + output_code: &str, + source_map: &str, ) -> std::io::Result<()> { let (cache_path, source_map_path) = self.cache_path(filename, source_code); // TODO(ry) This is a race condition w.r.t to exists() -- probably should @@ -148,8 +149,8 @@ impl DenoDir { if cache_path.exists() && source_map_path.exists() { Ok(()) } else { - fs::write(cache_path, output_code)?; - fs::write(source_map_path, source_map)?; + fs::write(cache_path, output_code.as_bytes())?; + fs::write(source_map_path, source_map.as_bytes())?; Ok(()) } } @@ -221,7 +222,7 @@ impl DenoDir { return Err(e.into()); } } - Ok(c) => c, + Ok(c) => String::from_utf8(c).unwrap(), }; // .mime file might not exists // this is okay for local source: maybe_content_type_str will be None @@ -318,7 +319,8 @@ impl DenoDir { out.source_code = filter_shebang(out.source_code); - let result = self.load_cache(out.filename.as_str(), &out.source_code); + let result = + self.load_cache(out.filename.as_str(), out.source_code.as_str()); match result { Err(err) => { if err.kind() == std::io::ErrorKind::NotFound { @@ -433,7 +435,7 @@ impl DenoDir { } impl SourceMapGetter for DenoDir { - fn get_source_map(&self, script_name: &str) -> Option> { + fn get_source_map(&self, script_name: &str) -> Option { match self.code_fetch(script_name, ".") { Err(_e) => { return None; @@ -468,21 +470,15 @@ fn get_cache_filename(basedir: &Path, url: &Url) -> PathBuf { } // https://github.com/denoland/deno/blob/golang/deno_dir.go#L25-L30 -fn source_code_hash(filename: &str, source_code: &[u8]) -> String { +fn source_code_hash(filename: &str, source_code: &str) -> String { let mut ctx = ring::digest::Context::new(&ring::digest::SHA1); ctx.update(filename.as_bytes()); - ctx.update(source_code); + ctx.update(source_code.as_bytes()); let digest = ctx.finish(); - let mut out = String::with_capacity(digest.as_ref().len() * 2); - const DIGIT_TO_CHAR: [char; 16] = [ - '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'a', 'b', 'c', 'd', 'e', - 'f', - ]; + let mut out = String::new(); + // TODO There must be a better way to do this... for byte in digest.as_ref() { - let low = (byte & 0x0F) as usize; - let high = (byte >> 4) as usize; - out.push(DIGIT_TO_CHAR[high]); - out.push(DIGIT_TO_CHAR[low]); + write!(&mut out, "{:02x}", byte).unwrap(); } out } @@ -543,15 +539,15 @@ fn map_content_type(path: &Path, content_type: Option<&str>) -> msg::MediaType { } } -fn filter_shebang(code: Vec) -> Vec { - if !code.starts_with(b"#!") { +fn filter_shebang(code: String) -> String { + if !code.starts_with("#!") { return code; } - if let Some(i) = code.iter().position(|&x| x == b'\n') { - let rest = &code[i..]; - rest.to_vec() + if let Some(i) = code.find('\n') { + let (_, rest) = code.split_at(i); + String::from(rest) } else { - Vec::new() + String::from("") } } @@ -604,7 +600,7 @@ mod tests { .path() .join("gen/a3e29aece8d35a19bf9da2bb1c086af71fb36ed5.js.map") ), - deno_dir.cache_path("hello.ts", b"1+2") + deno_dir.cache_path("hello.ts", "1+2") ); } @@ -613,9 +609,9 @@ mod tests { let (_temp_dir, deno_dir) = test_setup(); let filename = "hello.js"; - let source_code = b"1+2"; - let output_code = b"1+2 // output code"; - let source_map = b"{}"; + let source_code = "1+2"; + let output_code = "1+2 // output code"; + let source_map = "{}"; let (cache_path, source_map_path) = deno_dir.cache_path(filename, source_code); assert!( @@ -629,24 +625,24 @@ mod tests { let r = deno_dir.code_cache(filename, source_code, output_code, source_map); r.expect("code_cache error"); assert!(cache_path.exists()); - assert_eq!(output_code, &fs::read(&cache_path).unwrap()[..]); + assert_eq!(output_code, fs::read_to_string(&cache_path).unwrap()); } #[test] fn test_source_code_hash() { assert_eq!( "a3e29aece8d35a19bf9da2bb1c086af71fb36ed5", - source_code_hash("hello.ts", b"1+2") + source_code_hash("hello.ts", "1+2") ); // Different source_code should result in different hash. assert_eq!( "914352911fc9c85170908ede3df1128d690dda41", - source_code_hash("hello.ts", b"1") + source_code_hash("hello.ts", "1") ); // Different filename should result in different hash. assert_eq!( "2e396bc66101ecc642db27507048376d972b1b70", - source_code_hash("hi.ts", b"1+2") + source_code_hash("hi.ts", "1+2") ); } @@ -668,9 +664,10 @@ mod tests { println!("module_name {} filename {}", module_name, filename); assert!(result.is_ok()); let r = result.unwrap(); - let expected: &[u8] = - b"export { printHello } from \"./print_hello.ts\";\n"; - assert_eq!(r.source_code, expected); + assert_eq!( + &(r.source_code), + "export { printHello } from \"./print_hello.ts\";\n" + ); assert_eq!(&(r.media_type), &msg::MediaType::TypeScript); // Should not create .mime file due to matching ext assert!(fs::read_to_string(&mime_file_name).is_err()); @@ -680,9 +677,10 @@ mod tests { let result2 = deno_dir.get_source_code(module_name, &filename); assert!(result2.is_ok()); let r2 = result2.unwrap(); - let expected2: &[u8] = - b"export { printHello } from \"./print_hello.ts\";\n"; - assert_eq!(r2.source_code, expected2); + assert_eq!( + &(r2.source_code), + "export { printHello } from \"./print_hello.ts\";\n" + ); // If get_source_code does not call remote, this should be JavaScript // as we modified before! (we do not overwrite .mime due to no http fetch) assert_eq!(&(r2.media_type), &msg::MediaType::JavaScript); @@ -697,8 +695,7 @@ mod tests { let result3 = deno_dir.get_source_code(module_name, &filename); assert!(result3.is_ok()); let r3 = result3.unwrap(); - let expected3: &[u8] = - b"export { printHello } from \"./print_hello.ts\";\n"; + let expected3 = "export { printHello } from \"./print_hello.ts\";\n"; assert_eq!(r3.source_code, expected3); // Now the old .mime file should have gone! Resolved back to TypeScript assert_eq!(&(r3.media_type), &msg::MediaType::TypeScript); @@ -724,7 +721,7 @@ mod tests { println!("module_name {} filename {}", module_name, filename); assert!(result.is_ok()); let r = result.unwrap(); - let expected: &[u8] = b"export const loaded = true;\n"; + let expected = "export const loaded = true;\n"; assert_eq!(r.source_code, expected); // Mismatch ext with content type, create .mime assert_eq!(&(r.media_type), &msg::MediaType::JavaScript); @@ -738,7 +735,7 @@ mod tests { let result2 = deno_dir.get_source_code(module_name, &filename); assert!(result2.is_ok()); let r2 = result2.unwrap(); - let expected2: &[u8] = b"export const loaded = true;\n"; + let expected2 = "export const loaded = true;\n"; assert_eq!(r2.source_code, expected2); // If get_source_code does not call remote, this should be TypeScript // as we modified before! (we do not overwrite .mime due to no http fetch) @@ -754,7 +751,7 @@ mod tests { let result3 = deno_dir.get_source_code(module_name, &filename); assert!(result3.is_ok()); let r3 = result3.unwrap(); - let expected3: &[u8] = b"export const loaded = true;\n"; + let expected3 = "export const loaded = true;\n"; assert_eq!(r3.source_code, expected3); // Now the old .mime file should be overwritten back to JavaScript! // (due to http fetch) @@ -785,7 +782,7 @@ mod tests { let result = deno_dir.fetch_remote_source(module_name, &filename); assert!(result.is_ok()); let r = result.unwrap().unwrap(); - assert_eq!(&(r.source_code), b"export const loaded = true;\n"); + assert_eq!(&(r.source_code), "export const loaded = true;\n"); assert_eq!(&(r.media_type), &msg::MediaType::TypeScript); // matching ext, no .mime file created assert!(fs::read_to_string(&mime_file_name).is_err()); @@ -795,7 +792,7 @@ mod tests { let result2 = deno_dir.fetch_local_source(module_name, &filename); assert!(result2.is_ok()); let r2 = result2.unwrap().unwrap(); - assert_eq!(&(r2.source_code), b"export const loaded = true;\n"); + assert_eq!(&(r2.source_code), "export const loaded = true;\n"); // Not MediaType::TypeScript due to .mime modification assert_eq!(&(r2.media_type), &msg::MediaType::JavaScript); }); @@ -818,7 +815,7 @@ mod tests { let result = deno_dir.fetch_remote_source(module_name, &filename); assert!(result.is_ok()); let r = result.unwrap().unwrap(); - assert_eq!(&(r.source_code), b"export const loaded = true;\n"); + assert_eq!(&(r.source_code), "export const loaded = true;\n"); assert_eq!(&(r.media_type), &msg::MediaType::TypeScript); // no ext, should create .mime file assert_eq!( @@ -837,7 +834,7 @@ mod tests { let result_2 = deno_dir.fetch_remote_source(module_name_2, &filename_2); assert!(result_2.is_ok()); let r2 = result_2.unwrap().unwrap(); - assert_eq!(&(r2.source_code), b"export const loaded = true;\n"); + assert_eq!(&(r2.source_code), "export const loaded = true;\n"); assert_eq!(&(r2.media_type), &msg::MediaType::JavaScript); // mismatch ext, should create .mime file assert_eq!( @@ -857,7 +854,7 @@ mod tests { let result_3 = deno_dir.fetch_remote_source(module_name_3, &filename_3); assert!(result_3.is_ok()); let r3 = result_3.unwrap().unwrap(); - assert_eq!(&(r3.source_code), b"export const loaded = true;\n"); + assert_eq!(&(r3.source_code), "export const loaded = true;\n"); assert_eq!(&(r3.media_type), &msg::MediaType::TypeScript); // unknown ext, should create .mime file assert_eq!( @@ -880,7 +877,7 @@ mod tests { let result = deno_dir.fetch_local_source(module_name, &filename); assert!(result.is_ok()); let r = result.unwrap().unwrap(); - assert_eq!(&(r.source_code), b"export const loaded = true;\n"); + assert_eq!(&(r.source_code), "export const loaded = true;\n"); assert_eq!(&(r.media_type), &msg::MediaType::TypeScript); } @@ -1286,11 +1283,11 @@ mod tests { #[test] fn test_filter_shebang() { - assert_eq!(filter_shebang(b"".to_vec()), b""); - assert_eq!(filter_shebang(b"#".to_vec()), b"#"); - assert_eq!(filter_shebang(b"#!".to_vec()), b""); - assert_eq!(filter_shebang(b"#!\n\n".to_vec()), b"\n\n"); - let code = b"#!/usr/bin/env deno\nconsole.log('hello');\n".to_vec(); - assert_eq!(filter_shebang(code), b"\nconsole.log('hello');\n"); + assert_eq!(filter_shebang("".to_string()), ""); + assert_eq!(filter_shebang("#".to_string()), "#"); + assert_eq!(filter_shebang("#!".to_string()), ""); + assert_eq!(filter_shebang("#!\n\n".to_string()), "\n\n"); + let code = "#!/usr/bin/env deno\nconsole.log('hello');\n".to_string(); + assert_eq!(filter_shebang(code), "\nconsole.log('hello');\n"); } } diff --git a/src/http_util.rs b/src/http_util.rs index 3d8ae6791a1eb2..e3c21e0a9cf207 100644 --- a/src/http_util.rs +++ b/src/http_util.rs @@ -55,7 +55,7 @@ fn resolve_uri_from_location(base_uri: &Uri, location: &str) -> Uri { // The CodeFetch message is used to load HTTP javascript resources and expects a // synchronous response, this utility method supports that. -pub fn fetch_sync_string(module_name: &str) -> DenoResult<(Vec, String)> { +pub fn fetch_sync_string(module_name: &str) -> DenoResult<(String, String)> { let url = module_name.parse::().unwrap(); let client = get_client(); // TODO(kevinkassimo): consider set a max redirection counter @@ -93,11 +93,11 @@ pub fn fetch_sync_string(module_name: &str) -> DenoResult<(Vec, String)> { let body = response .into_body() .concat2() - .map(|body| body.to_vec()) + .map(|body| String::from_utf8(body.to_vec()).unwrap()) .map_err(DenoError::from); body.join(future::ok(content_type)) - }).and_then(|(body_bytes, maybe_content_type)| { - future::ok((body_bytes, maybe_content_type.unwrap())) + }).and_then(|(body_string, maybe_content_type)| { + future::ok((body_string, maybe_content_type.unwrap())) }); tokio_util::block_on(fetch_future) diff --git a/src/js_errors.rs b/src/js_errors.rs index 59e388162521ed..87b1db0d5a79fe 100644 --- a/src/js_errors.rs +++ b/src/js_errors.rs @@ -17,7 +17,7 @@ use std::collections::HashMap; pub trait SourceMapGetter { /// Returns the raw source map file. - fn get_source_map(&self, script_name: &str) -> Option>; + fn get_source_map(&self, script_name: &str) -> Option; } struct SourceMap { @@ -287,9 +287,7 @@ fn parse_map_string( } _ => match getter.get_source_map(script_name) { None => None, - Some(raw_source_map) => SourceMap::from_json( - &String::from_utf8(raw_source_map).expect("SourceMap is not utf-8"), - ), + Some(raw_source_map) => SourceMap::from_json(&raw_source_map), }, } } @@ -346,13 +344,13 @@ mod tests { struct MockSourceMapGetter {} impl SourceMapGetter for MockSourceMapGetter { - fn get_source_map(&self, script_name: &str) -> Option> { - let s: &[u8] = match script_name { - "foo_bar.ts" => br#"{"sources": ["foo_bar.ts"], "mappings":";;;IAIA,OAAO,CAAC,GAAG,CAAC,qBAAqB,EAAE,EAAE,CAAC,OAAO,CAAC,CAAC;IAC/C,OAAO,CAAC,GAAG,CAAC,eAAe,EAAE,IAAI,CAAC,QAAQ,CAAC,IAAI,CAAC,CAAC;IACjD,OAAO,CAAC,GAAG,CAAC,WAAW,EAAE,IAAI,CAAC,QAAQ,CAAC,EAAE,CAAC,CAAC;IAE3C,OAAO,CAAC,GAAG,CAAC,GAAG,CAAC,CAAC"}"#, - "bar_baz.ts" => br#"{"sources": ["bar_baz.ts"], "mappings":";;;IAEA,CAAC,KAAK,IAAI,EAAE;QACV,MAAM,GAAG,GAAG,sDAAa,OAAO,2BAAC,CAAC;QAClC,OAAO,CAAC,GAAG,CAAC,GAAG,CAAC,CAAC;IACnB,CAAC,CAAC,EAAE,CAAC;IAEQ,QAAA,GAAG,GAAG,KAAK,CAAC;IAEzB,OAAO,CAAC,GAAG,CAAC,GAAG,CAAC,CAAC"}"#, + fn get_source_map(&self, script_name: &str) -> Option { + let s = match script_name { + "foo_bar.ts" => r#"{"sources": ["foo_bar.ts"], "mappings":";;;IAIA,OAAO,CAAC,GAAG,CAAC,qBAAqB,EAAE,EAAE,CAAC,OAAO,CAAC,CAAC;IAC/C,OAAO,CAAC,GAAG,CAAC,eAAe,EAAE,IAAI,CAAC,QAAQ,CAAC,IAAI,CAAC,CAAC;IACjD,OAAO,CAAC,GAAG,CAAC,WAAW,EAAE,IAAI,CAAC,QAAQ,CAAC,EAAE,CAAC,CAAC;IAE3C,OAAO,CAAC,GAAG,CAAC,GAAG,CAAC,CAAC"}"#, + "bar_baz.ts" => r#"{"sources": ["bar_baz.ts"], "mappings":";;;IAEA,CAAC,KAAK,IAAI,EAAE;QACV,MAAM,GAAG,GAAG,sDAAa,OAAO,2BAAC,CAAC;QAClC,OAAO,CAAC,GAAG,CAAC,GAAG,CAAC,CAAC;IACnB,CAAC,CAAC,EAAE,CAAC;IAEQ,QAAA,GAAG,GAAG,KAAK,CAAC;IAEzB,OAAO,CAAC,GAAG,CAAC,GAAG,CAAC,CAAC"}"#, _ => return None, }; - Some(s.to_vec()) + Some(s.to_string()) } } diff --git a/src/msg.fbs b/src/msg.fbs index b576d0f23dd943..f68b40c1af8b4c 100644 --- a/src/msg.fbs +++ b/src/msg.fbs @@ -160,16 +160,18 @@ table CodeFetchRes { module_name: string; filename: string; media_type: MediaType; - source_code: [ubyte]; - output_code: [ubyte]; // Non-empty only if cached. - source_map: [ubyte]; // Non-empty only if cached. + // TODO These should be [ubyte]. + // See: https://github.com/denoland/deno/issues/1113 + source_code: string; + output_code: string; // Non-empty only if cached. + source_map: string; // Non-empty only if cached. } table CodeCache { filename: string; - source_code: [ubyte]; - output_code: [ubyte]; - source_map: [ubyte]; + source_code: string; + output_code: string; + source_map: string; } table Chdir { diff --git a/src/ops.rs b/src/ops.rs index 5b2598728ef8a2..638195442e0192 100644 --- a/src/ops.rs +++ b/src/ops.rs @@ -268,14 +268,14 @@ fn op_code_fetch( module_name: Some(builder.create_string(&out.module_name)), filename: Some(builder.create_string(&out.filename)), media_type: out.media_type, - source_code: Some(builder.create_vector(&out.source_code)), + source_code: Some(builder.create_string(&out.source_code)), ..Default::default() }; if let Some(ref output_code) = out.maybe_output_code { - msg_args.output_code = Some(builder.create_vector(output_code)); + msg_args.output_code = Some(builder.create_string(output_code)); } if let Some(ref source_map) = out.maybe_source_map { - msg_args.source_map = Some(builder.create_vector(source_map)); + msg_args.source_map = Some(builder.create_string(source_map)); } let inner = msg::CodeFetchRes::create(builder, &msg_args); Ok(serialize_response( @@ -305,7 +305,7 @@ fn op_code_cache( Box::new(futures::future::result(|| -> OpResult { state .dir - .code_cache(filename, &source_code, &output_code, &source_map)?; + .code_cache(filename, source_code, output_code, source_map)?; Ok(empty_buf()) }())) }