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

refactor: TS compiler and module graph #5817

Merged
merged 20 commits into from
May 29, 2020
Merged
Show file tree
Hide file tree
Changes from 6 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
74 changes: 74 additions & 0 deletions cli/global_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ use crate::deno_dir;
use crate::file_fetcher::SourceFileFetcher;
use crate::flags;
use crate::http_cache;
use crate::import_map::ImportMap;
use crate::lockfile::Lockfile;
use crate::module_graph::ModuleGraphLoader;
use crate::msg;
use crate::permissions::Permissions;
use crate::tsc::CompiledModule;
Expand Down Expand Up @@ -89,6 +91,78 @@ impl GlobalState {
Ok(GlobalState(Arc::new(inner)))
}

pub async fn prepare_module_load(
&self,
module_specifier: ModuleSpecifier,
maybe_referrer: Option<ModuleSpecifier>,
target_lib: TargetLib,
permissions: Permissions,
is_dyn_import: bool,
maybe_import_map: Option<ImportMap>,
) -> Result<(), ErrBox> {
let state1 = self.clone();
let state2 = self.clone();
let module_specifier = module_specifier.clone();

// TODO(ry) Try to lift compile_lock as high up in the call stack for
// sanity.
let compile_lock = self.compile_lock.lock().await;

let mut module_graph_loader = ModuleGraphLoader::new(
self.file_fetcher.clone(),
maybe_import_map,
permissions.clone(),
is_dyn_import,
false,
);
module_graph_loader
.add_to_graph(&module_specifier, maybe_referrer)
.await?;
let module_graph = module_graph_loader.get_graph();

let out = self
.file_fetcher
.fetch_cached_source_file(&module_specifier, permissions.clone())
.expect("Source file not found");

// Check if we need to compile files
match out.media_type {
msg::MediaType::TypeScript
| msg::MediaType::TSX
| msg::MediaType::JSX => {
state1
.ts_compiler
.new_compile(
state1.clone(),
&out,
target_lib,
permissions,
module_graph,
)
.await?;
}
msg::MediaType::JavaScript => {
if state1.ts_compiler.compile_js {
state2
.ts_compiler
.new_compile(
state1.clone(),
&out,
target_lib,
permissions,
module_graph,
)
.await?;
}
}
_ => {}
};

drop(compile_lock);

Ok(())
}

pub async fn fetch_compiled_module(
&self,
module_specifier: ModuleSpecifier,
Expand Down
36 changes: 28 additions & 8 deletions cli/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -388,14 +388,15 @@ async fn bundle_command(
source_file: String,
out_file: Option<PathBuf>,
) -> Result<(), ErrBox> {
let mut module_name = ModuleSpecifier::resolve_url_or_path(&source_file)?;
let url = module_name.as_url();
let mut module_specifier =
ModuleSpecifier::resolve_url_or_path(&source_file)?;
let url = module_specifier.as_url();

// TODO(bartlomieju): fix this hack in ModuleSpecifier
if url.scheme() == "file" {
let a = deno_fs::normalize_path(&url.to_file_path().unwrap());
let u = Url::from_file_path(a).unwrap();
module_name = ModuleSpecifier::from(u)
module_specifier = ModuleSpecifier::from(u)
}

debug!(">>>>> bundle START");
Expand All @@ -413,18 +414,37 @@ async fn bundle_command(

let global_state = GlobalState::new(flags)?;

let bundle_result = tsc::bundle(
// TODO(bartlomieju): probably should respect --quiet flag
eprintln!("Bundling {}", module_specifier.to_string());

let output = tsc::bundle(
&global_state,
compiler_config,
module_name,
module_specifier,
maybe_import_map,
out_file,
global_state.flags.unstable,
)
.await;
.await?;

debug!(">>>>> bundle END");
bundle_result

let output_string = fmt::format_text(&output)?;

if let Some(out_file_) = out_file.as_ref() {
eprintln!("Emitting bundle to {:?}", out_file_);

let output_bytes = output_string.as_bytes();
let output_len = output_bytes.len();

deno_fs::write_file(out_file_, output_bytes, 0o666)?;
// TODO(bartlomieju): do we really need to show this info? (it doesn't respect --quiet flag)
// TODO(bartlomieju): add "humanFileSize" method
eprintln!("{} bytes emitted.", output_len);
} else {
println!("{}", output_string);
}

Ok(())
}

async fn doc_command(
Expand Down
15 changes: 6 additions & 9 deletions cli/module_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::op_error::OpError;
use crate::permissions::Permissions;
use crate::swc_util::analyze_dependencies_and_references;
use crate::swc_util::TsReferenceKind;
use crate::tsc::get_available_libs;
use crate::tsc::AVAILABLE_LIBS;
use deno_core::ErrBox;
use deno_core::ModuleSpecifier;
use futures::stream::FuturesUnordered;
Expand Down Expand Up @@ -138,8 +138,9 @@ impl ModuleGraphLoader {
pub async fn add_to_graph(
&mut self,
specifier: &ModuleSpecifier,
maybe_referrer: Option<ModuleSpecifier>,
) -> Result<(), ErrBox> {
self.download_module(specifier.clone(), None)?;
self.download_module(specifier.clone(), maybe_referrer)?;

loop {
let (specifier, source_file) =
Expand Down Expand Up @@ -238,10 +239,8 @@ impl ModuleGraphLoader {
imports.push(import_descriptor);
}

let available_libs = get_available_libs();

for ref_desc in ref_descs {
if available_libs.contains(&ref_desc.specifier) {
if AVAILABLE_LIBS.contains(&ref_desc.specifier.as_str()) {
continue;
}

Expand Down Expand Up @@ -465,10 +464,8 @@ impl ModuleGraphLoader {
imports.push(import_descriptor);
}

let available_libs = get_available_libs();

for ref_desc in ref_descs {
if available_libs.contains(&ref_desc.specifier) {
if AVAILABLE_LIBS.contains(&ref_desc.specifier.as_str()) {
continue;
}

Expand Down Expand Up @@ -537,7 +534,7 @@ mod tests {
false,
false,
);
graph_loader.add_to_graph(&module_specifier).await?;
graph_loader.add_to_graph(&module_specifier, None).await?;
Ok(graph_loader.get_graph())
}

Expand Down
51 changes: 36 additions & 15 deletions cli/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -357,22 +357,43 @@ impl ModuleLoader for State {
fn prepare_load(
&self,
_load_id: ModuleLoadId,
_module_specifier: &ModuleSpecifier,
_maybe_referrer: Option<String>,
_is_dyn_import: bool,
module_specifier: &ModuleSpecifier,
maybe_referrer: Option<String>,
is_dyn_import: bool,
) -> Pin<Box<dyn Future<Output = Result<(), ErrBox>>>> {
// TODO(bartlomieju):
// 1. recursively:
// a) resolve specifier
// b) check permission if dynamic import
// c) fetch/download source code
// d) parse the source code and extract all import/exports (dependencies)
// e) add discovered deps and loop algorithm until no new dependencies
// are discovered
// 2. run through appropriate compiler giving it access only to
// discovered files

async { Ok(()) }.boxed_local()
let module_specifier = module_specifier.clone();
let state = self.borrow();
let target_lib = state.target_lib.clone();
let maybe_import_map = state.import_map.clone();
let permissions = if state.is_main && !is_dyn_import {
Permissions::allow_all()
} else {
state.permissions.clone()
};
let global_state = state.global_state.clone();
Copy link
Member Author

@bartlomieju bartlomieju May 25, 2020

Choose a reason for hiding this comment

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

Implementation of this method requires to solve issue described in #1692 as well.

Currently TS compiler is spun up during fetch_compiled_module() call - if root module was already compiled and hasn't changed then cached code will be used. If one of root's dependencies has changed then TS compiler will be spun up.

Implementing prepare_load() means that we need to manually handle outdated dependencies - because prepare_load() is the only function that can spin up TS compiler. load() will only access already compiled source code in $DENO_DIR, but it won't be able to spin up TS compiler.

CC @kitsonk @ry

Copy link
Member Author

Choose a reason for hiding this comment

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

Added basic handling of cache invalidation by saving dependency list in "module graph" meta file.

// TODO(bartlomieju): I'm not sure if it's correct to ignore
// bad referrer - this is the case for `Deno.core.evalContext()` where
// `ref_str` is `<unknown>`.
let maybe_referrer = if let Some(ref_str) = maybe_referrer {
ModuleSpecifier::resolve_url(&ref_str).ok()
} else {
None
};
drop(state);

async move {
global_state
.prepare_module_load(
module_specifier,
maybe_referrer,
target_lib,
permissions,
is_dyn_import,
maybe_import_map,
)
.await
}
.boxed_local()
}
}

Expand Down
2 changes: 1 addition & 1 deletion cli/tests/error_005_missing_dynamic_import.ts.out
Original file line number Diff line number Diff line change
@@ -1 +1 @@
error: Cannot resolve module "[WILDCARD]/bad-module.ts" from "[WILDCARD]/error_005_missing_dynamic_import.ts"
error: Uncaught TypeError: Cannot resolve module "[WILDCARD]/bad-module.ts" from "[WILDCARD]/error_005_missing_dynamic_import.ts"
3 changes: 2 additions & 1 deletion cli/tests/error_012_bad_dynamic_import_specifier.ts.out
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
error: relative import path "bad-module.ts" not prefixed with / or ./ or ../ Imported from "[WILDCARD]/error_012_bad_dynamic_import_specifier.ts"
Compile [WILDCARD]error_012_bad_dynamic_import_specifier.ts
error: Uncaught TypeError: relative import path "bad-module.ts" not prefixed with / or ./ or ../ Imported from "[WILDCARD]/error_012_bad_dynamic_import_specifier.ts"
5 changes: 1 addition & 4 deletions cli/tests/error_syntax.js.out
Original file line number Diff line number Diff line change
@@ -1,4 +1 @@
error: Uncaught SyntaxError: Unexpected identifier
(the following is a syntax error ^^ ! )
~~~~~~~~~
at [WILDCARD]tests/error_syntax.js:3:6
error: Expected Comma, got Some(Word(following)) at [WILDCARD]tests/error_syntax.js:3:5
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to improve these over time, as got Some(Word(follow)) is pretty inaccessible to developers. Ultimately we want go grab the source line and highlight the span, and put some sort of match in to humanise the errors.

3 changes: 1 addition & 2 deletions cli/tests/error_syntax_empty_trailing_line.mjs.out
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
error: Uncaught SyntaxError: Unexpected end of input
at [WILDCARD]tests/error_syntax_empty_trailing_line.mjs:[WILDCARD]
error: Unexpected eof at [WILDCARD]tests/error_syntax_empty_trailing_line.mjs:2:21
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the same here... "eof" is sometimes well understood, but "Unexpected end of file" is clearer. No sense in holding up this PR, but maybe we should have a DX issue for error messages for this. Hopefully something the wider community would help out with.

4 changes: 2 additions & 2 deletions cli/tests/integration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1359,7 +1359,7 @@ itest!(error_004_missing_module {
});

itest!(error_005_missing_dynamic_import {
args: "run --reload --allow-read error_005_missing_dynamic_import.ts",
args: "run --reload --allow-read --quiet error_005_missing_dynamic_import.ts",
exit_code: 1,
output: "error_005_missing_dynamic_import.ts.out",
});
Expand Down Expand Up @@ -1406,7 +1406,7 @@ itest!(error_014_catch_dynamic_import_error {
});

itest!(error_015_dynamic_import_permissions {
args: "run --reload error_015_dynamic_import_permissions.js",
args: "run --reload --quiet error_015_dynamic_import_permissions.js",
output: "error_015_dynamic_import_permissions.out",
exit_code: 1,
http_server: true,
Expand Down
Loading