refactor: rewrite TS dependency analysis in Rust #5029

bartlomieju · 2020-05-01T14:40:12Z

This PR completely overhauls how module analysis is performed in TS compiler by moving the logic to Rust.

In the current setup module analysis is performed using ts.preProcessFile API in a special TS compiler worker running on a separate thread.

ts.preProcessFile allowed us to build a lot of functionality in CLI including X-TypeScript-Types header support and @deno-types directive support. Unfortunately at the same time complexity of the ops required to perform supporting tasks exploded and caused some hidden permission escapes.

This PR introduces ModuleGraphLoader which can parse source and load recursively all dependent source files; as well as declaration files. All dependencies used in TS compiler and now fetched and collected upfront in Rust before spinning up TS compiler.

To achieve feature parity with existing APIs this PR includes a lot of changes:

add ModuleGraphLoader
- can fetch local and remote sources
- parses source code using SWC and extracts imports, exports, file references, special headers
- this struct inherited all of the hidden complexity and cruft from TS version and requires several follow up PRs
rewrite cli/tsc.rs to perform module analysis upfront and send all required source code to TS worker in one message
remove op_resolve_modules and op_fetch_source_files from cli/ops/compiler.rs
run TS worker on the same thread

bartlomieju · 2020-05-01T16:37:01Z

@kitsonk I won't be able to finish this PR tonight 😞 feel free to pick up

bartlomieju · 2020-05-06T12:56:23Z

Note to self:

ModuleGraph should be extended to understand X-TypeScript-Types headers as well as type, reference and lib directives (/// <reference lib="dom">)
ModuleGraphSourceFile should utilize these directives to download references files
ModuleGraphSourceFile should contain following fields:
- imports - additionally there should be annotation if import has respective type declaration based on X-TypeScript-Types header or // @deno-types="./foo.d.ts" directive above import statement
- lib_references_directives
- type_reference_directives
- referenced_files
After ModuleGraph is constructed and all sources loaded it should be possible to JSON-serialize the graph with all of it sources so it can be fed into TS compiler in a single message
It would be best if ModuleGraph could collect information about module resolution so TS compiler doesn't need op_resolve_modules - it's only used to build cache of resolved specifier for unresolved specifier/referrer pair (so import could be described something like { specifier: "./foo.ts", resolvedSpecifier: "file://a/b/c/foo.ts, type: "sourceFile" }).

bartlomieju · 2020-05-17T20:26:03Z

I thought that after this change running target/release/deno info https://deno.land/x/oak/mod.ts would not invoke the TS compiler, but I still see Compile https://deno.land/x/oak/mod.ts output.

I did not change deno info in this PR - it's still wired to TS compiler. I'll see to that in follow up PRs this one is big enough.

ry

LGTM - better to land this - it's clearly an improvement. I'm not super happy with the memory:// stuff, but that can be dealt with later.

kitsonk · 2020-05-18T00:43:20Z

FYI, I've got time to do a proper review right now. Hopefully a few minutes.

kitsonk

A few comments but LGTM, though hard to fully tell, but certainly something that can be iterated on.

One thing that is/was happening that we need to check after this is that sometimes we would have a miss with dynamic imports that TypeScript would get, but ts.preProcessFile() would miss... like:

const spec = "./foo.ts";
const foo = await import(spec);

I don't know if this catches those or not. If not, we still need an op for TypeScript to pull in something we couldn't statically identify upfront.

cli/js/compiler.ts

kitsonk · 2020-05-18T00:48:19Z

cli/js/compiler.ts

@@ -1327,7 +1184,6 @@ async function compile(
      rootNames,
      options,
      host,
-      oldProgram: TS_SNAPSHOT_PROGRAM,


👍 as I said above, we should document why we have an unused const above.

cli/js/compiler.ts

kitsonk · 2020-05-18T00:53:11Z

cli/module_graph.rs

+        spec
+      } else {
+        ModuleSpecifier::resolve_url(&format!("memory://{}", specifier))?
+      };


Hrmmmm... yeah, don't like this easier, but yeah... maybe a TODO so we remember to revisit.

kitsonk · 2020-05-18T00:55:58Z

cli/ops/compiler.rs

+pub fn init(i: &mut CoreIsolate, _s: &State) {
+  let custom_assets = std::collections::HashMap::new();
+  // TODO(ry) use None.
+  // TODO(bartlomieju): is this op even required?


It could not be required... but it means we need to catch these dependencies when using lib with the runtime compiler APIs.

Not sure I understand, I think if we loaded all available libs during snapshot it could be removed

oh, yeah I forgot we did that...

bartlomieju · 2020-05-18T09:53:30Z

One thing that is/was happening that we need to check after this is that sometimes we would have a miss with dynamic imports that TypeScript would get, but ts.preProcessFile() would miss... like:
const spec = "./foo.ts";
const foo = await import(spec);
I don't know if this catches those or not. If not, we still need an op for TypeScript to pull in something we couldn't statically identify upfront.

So in my PR dynamic imports are only parsed in case of bundle, and only those dynamic imports that look like import("./some/specifier.ts"). What you shown above it probably doable but it won't work after this PR.

garronej · 2020-05-21T17:40:33Z

This is the commit that breaks EVT.

This commit fixes a bug introduced in #5029 that caused bad handling of redirects during module analysis. Also ensured that duplicate modules are not downloaded.

This PR addresses many problems with module graph loading introduced in #5029, as well as many long standing issues. "ModuleGraphLoader" has been wired to "ModuleLoader" implemented on "State" - that means that dependency analysis and fetching is done before spinning up TS compiler worker. Basic dependency tracking for TS compilation has been implemented. Errors caused by import statements are now annotated with import location. Co-authored-by: Ryan Dahl <ry@tinyclouds.org>

This PR addresses many problems with module graph loading introduced in denoland#5029, as well as many long standing issues. "ModuleGraphLoader" has been wired to "ModuleLoader" implemented on "State" - that means that dependency analysis and fetching is done before spinning up TS compiler worker. Basic dependency tracking for TS compilation has been implemented. Errors caused by import statements are now annotated with import location. Co-authored-by: Ryan Dahl <ry@tinyclouds.org>

bartlomieju force-pushed the module_graph branch 7 times, most recently from 9b4ab2f to 691715a Compare May 11, 2020 00:17

bartlomieju added 21 commits May 11, 2020 13:19

add permissions argument to file fetcher methods

8a2d4e2

drop permissions prefix

c7dc065

add permission check to SourceFileFetcher

a8f431d

use Permissions::allow_all

17d06b3

propagate proper permissions

38ebecd

add docs

98984e4

remove cli/compilers/js.rs

62e6db4

remove cli/compilers/; add cli/tsc.rs

161a361

prototype module graph loader

c1e55bc

add test for circular deps

ea6e891

update tests

198166a

prototype tests

cf840fb

parse imports and type directives

22e879f

type directives analysis

ac3668c

lint

d6aff93

basic compilation working

319b94a

fmt

db5405a

fix bundle tests

9bbd7a5

fixes

cfdef1e

fix integration tests

7827a56

fmt

ae1d693

review

fa3c816

fix

28544c9

ry approved these changes May 18, 2020

View reviewed changes

kitsonk reviewed May 18, 2020

View reviewed changes

bartlomieju added 4 commits May 18, 2020 11:58

review2

35adb52

Merge branch 'master' into module_graph

f84e66c

fmt

5d02e05

reset CI

aa67332

bartlomieju merged commit 9d63772 into denoland:master May 18, 2020

bartlomieju deleted the module_graph branch May 18, 2020 10:59

This was referenced May 19, 2020

Remove processing of files from TS compiler #5018

Closed

404 on importing type declarations files from pika.dev using Deno 1.0.1 #5696

Closed

v0.35.0 TSX file can't be compiled #4197

Closed

garronej mentioned this pull request May 21, 2020

TS type imports broken in v1.0.1 #5704

Closed

This was referenced May 22, 2020

fix: redirects handling in module analysis #5726

Merged

fix: ts type imports #5733

Merged

bartlomieju mentioned this pull request May 24, 2020

Dynamic import failing with TS5012 and TS6053 #4720

Closed

ry mentioned this pull request May 25, 2020

Bigger refactors #5858

Closed

6 tasks

njbraun mentioned this pull request May 26, 2020

404 when importing type declaration files from pika.dev using Deno 1.0.2 #5876

Closed

This was referenced May 26, 2020

Not able to import cjs files anymore #5878

Closed

Unable to import Javascript files from TSX #5900

Closed

refactor: TS compiler and module graph #5817

Merged

bartlomieju mentioned this pull request Jun 3, 2020

Getting "error: Read-only file system" for .graph cache file #6080

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: rewrite TS dependency analysis in Rust #5029

refactor: rewrite TS dependency analysis in Rust #5029

bartlomieju commented May 1, 2020 •

edited

Loading

bartlomieju commented May 1, 2020

bartlomieju commented May 6, 2020 •

edited

Loading

bartlomieju commented May 17, 2020

ry left a comment

kitsonk commented May 18, 2020

kitsonk left a comment

kitsonk May 18, 2020

kitsonk May 18, 2020

kitsonk May 18, 2020

bartlomieju May 18, 2020

kitsonk May 18, 2020

bartlomieju commented May 18, 2020

garronej commented May 21, 2020

refactor: rewrite TS dependency analysis in Rust #5029

refactor: rewrite TS dependency analysis in Rust #5029

Conversation

bartlomieju commented May 1, 2020 • edited Loading

bartlomieju commented May 1, 2020

bartlomieju commented May 6, 2020 • edited Loading

bartlomieju commented May 17, 2020

ry left a comment

Choose a reason for hiding this comment

kitsonk commented May 18, 2020

kitsonk left a comment

Choose a reason for hiding this comment

kitsonk May 18, 2020

Choose a reason for hiding this comment

kitsonk May 18, 2020

Choose a reason for hiding this comment

kitsonk May 18, 2020

Choose a reason for hiding this comment

bartlomieju May 18, 2020

Choose a reason for hiding this comment

kitsonk May 18, 2020

Choose a reason for hiding this comment

bartlomieju commented May 18, 2020

garronej commented May 21, 2020

bartlomieju commented May 1, 2020 •

edited

Loading

bartlomieju commented May 6, 2020 •

edited

Loading