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

fix: use inline source maps when present in js #8995

Merged
merged 10 commits into from
Jan 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .dprintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
"cli/dts/lib.webworker*.d.ts",
"cli/dts/typescript.d.ts",
"cli/tests/encoding",
"cli/tests/inline_js_source_map*",
"cli/tsc/*typescript.js",
"test_util/wpt",
"gh-pages",
Expand Down
3 changes: 2 additions & 1 deletion cli/ops/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,12 @@ fn op_apply_source_map(
let mut mappings_map: CachedMaps = HashMap::new();
let program_state = state.borrow::<Arc<ProgramState>>().clone();

let (orig_file_name, orig_line_number, orig_column_number) =
let (orig_file_name, orig_line_number, orig_column_number, _) =
get_orig_position(
args.file_name,
args.line_number.into(),
args.column_number.into(),
None,
&mut mappings_map,
program_state,
);
Expand Down
40 changes: 23 additions & 17 deletions cli/program_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,24 +300,10 @@ impl SourceMapGetter for ProgramState {
maybe_map
} else {
let code = String::from_utf8(code).unwrap();
let lines: Vec<&str> = code.split('\n').collect();
if let Some(last_line) = lines.last() {
if last_line
.starts_with("//# sourceMappingURL=data:application/json;base64,")
{
let input = last_line.trim_start_matches(
"//# sourceMappingURL=data:application/json;base64,",
);
let decoded_map = base64::decode(input)
.expect("Unable to decode source map from emitted file.");
Some(decoded_map)
} else {
None
}
} else {
None
}
source_map_from_code(code)
}
} else if let Ok(source) = self.load(specifier, None) {
source_map_from_code(source.code)
} else {
None
}
Expand Down Expand Up @@ -345,6 +331,26 @@ impl SourceMapGetter for ProgramState {
}
}

fn source_map_from_code(code: String) -> Option<Vec<u8>> {
let lines: Vec<&str> = code.split('\n').collect();
if let Some(last_line) = lines.last() {
if last_line
.starts_with("//# sourceMappingURL=data:application/json;base64,")
{
let input = last_line.trim_start_matches(
"//# sourceMappingURL=data:application/json;base64,",
);
let decoded_map = base64::decode(input)
.expect("Unable to decode source map from emitted file.");
Some(decoded_map)
} else {
None
}
} else {
None
}
}

#[test]
fn thread_safe() {
fn f<S: Send + Sync>(_: S) {}
Expand Down
82 changes: 53 additions & 29 deletions cli/source_maps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,15 @@ pub fn apply_source_map<G: SourceMapGetter>(
// prepareStackTrace().
let mut mappings_map: CachedMaps = HashMap::new();

let (script_resource_name, line_number, start_column) =
let (script_resource_name, line_number, start_column, source_line) =
get_maybe_orig_position(
js_error.script_resource_name.clone(),
js_error.line_number,
// start_column is 0-based, we need 1-based here.
js_error.start_column.map(|n| n + 1),
js_error.source_line.clone(),
&mut mappings_map,
getter.clone(),
getter,
);
let start_column = start_column.map(|n| n - 1);
// It is better to just move end_column to be the same distance away from
Expand All @@ -56,20 +57,6 @@ pub fn apply_source_map<G: SourceMapGetter>(
}
_ => None,
};
// if there is a source line that we might be different in the source file, we
// will go fetch it from the getter
let source_line = match line_number {
Some(ln)
if js_error.source_line.is_some() && script_resource_name.is_some() =>
{
getter.get_source_line(
&js_error.script_resource_name.clone().unwrap(),
// Getter expects 0-based line numbers, but ours are 1-based.
ln as usize - 1,
)
}
_ => js_error.source_line.clone(),
};

JsError {
message: js_error.message.clone(),
Expand All @@ -87,28 +74,43 @@ fn get_maybe_orig_position<G: SourceMapGetter>(
file_name: Option<String>,
line_number: Option<i64>,
column_number: Option<i64>,
source_line: Option<String>,
mappings_map: &mut CachedMaps,
getter: Arc<G>,
) -> (Option<String>, Option<i64>, Option<i64>) {
) -> (Option<String>, Option<i64>, Option<i64>, Option<String>) {
match (file_name, line_number, column_number) {
(Some(file_name_v), Some(line_v), Some(column_v)) => {
let (file_name, line_number, column_number) =
get_orig_position(file_name_v, line_v, column_v, mappings_map, getter);
(Some(file_name), Some(line_number), Some(column_number))
let (file_name, line_number, column_number, source_line) =
get_orig_position(
file_name_v,
line_v,
column_v,
source_line,
mappings_map,
getter,
);
(
Some(file_name),
Some(line_number),
Some(column_number),
source_line,
)
}
_ => (None, None, None),
_ => (None, None, None, source_line),
}
}

pub fn get_orig_position<G: SourceMapGetter>(
file_name: String,
line_number: i64,
column_number: i64,
source_line: Option<String>,
mappings_map: &mut CachedMaps,
getter: Arc<G>,
) -> (String, i64, i64) {
let maybe_source_map = get_mappings(&file_name, mappings_map, getter);
let default_pos = (file_name, line_number, column_number);
) -> (String, i64, i64, Option<String>) {
let maybe_source_map = get_mappings(&file_name, mappings_map, getter.clone());
let default_pos =
(file_name, line_number, column_number, source_line.clone());

// Lookup expects 0-based line and column numbers, but ours are 1-based.
let line_number = line_number - 1;
Expand All @@ -121,11 +123,33 @@ pub fn get_orig_position<G: SourceMapGetter>(
None => default_pos,
Some(token) => match token.get_source() {
None => default_pos,
Some(original) => (
original.to_string(),
i64::from(token.get_src_line()) + 1,
i64::from(token.get_src_col()) + 1,
),
Some(original) => {
let maybe_source_line =
if let Some(source_view) = token.get_source_view() {
source_view.get_line(token.get_src_line())
} else {
None
};

let source_line = if let Some(source_line) = maybe_source_line {
Some(source_line.to_string())
} else if let Some(source_line) = getter.get_source_line(
original,
// Getter expects 0-based line numbers, but ours are 1-based.
token.get_src_line() as usize,
) {
Some(source_line)
} else {
source_line
};

(
original.to_string(),
i64::from(token.get_src_line()) + 1,
i64::from(token.get_src_col()) + 1,
source_line,
)
}
},
}
}
Expand Down
2 changes: 2 additions & 0 deletions cli/tests/059_fs_relative_path_perm.ts.out
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
[WILDCARD]error: Uncaught PermissionDenied: read access to "non-existent", run again with the --allow-read flag
throw new ErrorClass(res.err.message);
^
at [WILDCARD]
2 changes: 2 additions & 0 deletions cli/tests/073_worker_error.ts.out
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,6 @@
at foo ([WILDCARD])
at [WILDCARD]
error: Uncaught (in promise) Error: Unhandled error event reached main worker.
throw new Error("Unhandled error event reached main worker.");
^
lucacasonato marked this conversation as resolved.
Show resolved Hide resolved
at Worker.#poll ([WILDCARD])
2 changes: 2 additions & 0 deletions cli/tests/074_worker_nested_error.ts.out
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,6 @@
at foo ([WILDCARD])
at [WILDCARD]
error: Uncaught (in promise) Error: Unhandled error event reached main worker.
throw new Error("Unhandled error event reached main worker.");
^
at Worker.#poll ([WILDCARD])
2 changes: 1 addition & 1 deletion cli/tests/fmt/expected_fmt_check_tests_dir.out
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
[WILDCARD]
error: Found 2 not formatted files in [WILDCARD] files
error: Found 5 not formatted files in [WILDCARD] files
6 changes: 6 additions & 0 deletions cli/tests/inline_js_source_map.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
1 + 1;
interface Test {
hello: string;
}

// throw new Error("Hello world!" as string);
4 changes: 4 additions & 0 deletions cli/tests/inline_js_source_map_2.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions cli/tests/inline_js_source_map_2.js.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
error: Uncaught Error: Hello world!
throw new Error("Hello world!");
^
at http://localhost:4545/cli/tests/inline_js_source_map_2.ts:6:7
6 changes: 6 additions & 0 deletions cli/tests/inline_js_source_map_2.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
1 + 1;
interface Test {
hello: string;
}

throw new Error("Hello world!" as unknown as string);
4 changes: 4 additions & 0 deletions cli/tests/inline_js_source_map_2_with_inline_contents.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions cli/tests/inline_js_source_map_2_with_inline_contents.js.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
error: Uncaught Error: Hello world!
throw new Error("Hello world!" as unknown as string);
^
at http://localhost:4545/cli/tests/inline_js_source_map_2.ts:6:7
4 changes: 4 additions & 0 deletions cli/tests/inline_js_source_map_with_contents_from_graph.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
error: Uncaught Error: Hello world!
// throw new Error("Hello world!" as string);
lucacasonato marked this conversation as resolved.
Show resolved Hide resolved
^
at http://localhost:4545/cli/tests/inline_js_source_map.ts:6:7
35 changes: 35 additions & 0 deletions cli/tests/integration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3446,6 +3446,41 @@ itest!(local_sources_not_cached_in_memory {
output: "no_mem_cache.js.out",
});

// This test checks that inline source map data is used. It uses a hand crafted
// source map that maps to a file that exists, but is not loaded into the module
// graph (inline_js_source_map_2.ts) (because there are no direct dependencies).
// Source line is not remapped because no inline source contents are included in
// the sourcemap and the file is not present in the dependency graph.
itest!(inline_js_source_map_2 {
args: "run --quiet inline_js_source_map_2.js",
output: "inline_js_source_map_2.js.out",
exit_code: 1,
});

// This test checks that inline source map data is used. It uses a hand crafted
// source map that maps to a file that exists, but is not loaded into the module
// graph (inline_js_source_map_2.ts) (because there are no direct dependencies).
// Source line remapped using th inline source contents that are included in the
// inline source map.
itest!(inline_js_source_map_2_with_inline_contents {
args: "run --quiet inline_js_source_map_2_with_inline_contents.js",
output: "inline_js_source_map_2_with_inline_contents.js.out",
exit_code: 1,
});

// This test checks that inline source map data is used. It uses a hand crafted
// source map that maps to a file that exists, and is loaded into the module
// graph because of a direct import statement (inline_js_source_map.ts). The
// source map was generated from an earlier version of this file, where the throw
// was not commented out. The source line is remapped using source contents that
// from the module graph.
itest!(inline_js_source_map_with_contents_from_graph {
args: "run --quiet inline_js_source_map_with_contents_from_graph.js",
output: "inline_js_source_map_with_contents_from_graph.js.out",
exit_code: 1,
http_server: true,
});

#[test]
fn cafile_env_fetch() {
use deno_core::url::Url;
Expand Down