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

Restore runtime lib generation capability #2868

Closed
kitsonk opened this issue Sep 5, 2019 · 9 comments
Closed

Restore runtime lib generation capability #2868

kitsonk opened this issue Sep 5, 2019 · 9 comments

Comments

@kitsonk
Copy link
Contributor

kitsonk commented Sep 5, 2019

With the recent changes to the build system, we removed the ability to generate the runtime type library based on the runtime code from Deno. We should restore this, so that we can automatically keep our type library in sync with the code.

This is something I am working on, but essentially we need to create a module that can be transpiled and injected into V8 at buildtime which sort of mirrors the process of the previous library builder which ran under Node.js. Because at the moment, there is no easy way to describe exactly what we do at the runtime (creating a window object that equals the global scope and injecting a namespace of Deno into the runtime) we have to do some TypeScript AST manipulation in order to get output that is valid. Previously we had used a library (ts-morph) to accomplish this, but that at this stage it is a bit too difficult to get running under the "bare" V8 we have at build time (or Deno for that matter) so we need to replicate the logic of the old builder but manipulate the TypeScript AST directly.

@ry
Copy link
Member

ry commented Sep 6, 2019

Yes, we should restore this.

I left a little hook, which I hope can help here. The d.ts file that is outputted by typescript is stored in deno_cli_snapshot::CLI_SNAPSHOT_DTS defined here:

pub static CLI_SNAPSHOT_DTS: &[u8] =
include_bytes!(concat!(env!("OUT_DIR"), "/CLI_SNAPSHOT.d.ts"));

It would be very awesome if we could simply use this as the lib.deno_runtime.d.ts without additional machinery... but it's not quite right. I wonder if by somehow shifting around the various exports and imports and declare globals if it wouldn't be possible to make this CLI_SNAPSHOT_DTS in the form we need.

If we find that is impossible, the second best thing would be to use CLI_SNAPSHOT_DTS as input to some custom AST manipulation machinery (something like ts_library_builder) to output the required lib.deno_runtime.d.ts.

@ry
Copy link
Member

ry commented Sep 6, 2019

Specifically - this would be the ideal patch:

From deda9bd79f1cba36b003e2da91edd9668c50bc61 Mon Sep 17 00:00:00 2001
From: Ryan Dahl <[email protected]>
Date: Thu, 5 Sep 2019 20:14:51 -0400
Subject: [PATCH] Ideal lib.deno_runtime.d.ts restoration

---
 cli/assets.rs        | 4 +---
 cli_snapshots/lib.rs | 8 ++++----
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/cli/assets.rs b/cli/assets.rs
index a0abca11..263f7100 100644
--- a/cli/assets.rs
+++ b/cli/assets.rs
@@ -1,8 +1,6 @@
-static DENO_RUNTIME: &str = include_str!("../js/lib.deno_runtime.d.ts");
-
 pub fn get_source_code(name: &str) -> Option<&'static str> {
   match name {
-    "lib.deno_runtime.d.ts" => Some(DENO_RUNTIME),
+    "lib.deno_runtime.d.ts" => Some(deno_cli_snapshots::CLI_SNAPSHOT_DTS),
     _ => deno_typescript::get_asset(name),
   }
 }
diff --git a/cli_snapshots/lib.rs b/cli_snapshots/lib.rs
index 1147e790..ff028cb0 100644
--- a/cli_snapshots/lib.rs
+++ b/cli_snapshots/lib.rs
@@ -2,15 +2,15 @@ pub static CLI_SNAPSHOT: &[u8] =
   include_bytes!(concat!(env!("OUT_DIR"), "/CLI_SNAPSHOT.bin"));
 pub static CLI_SNAPSHOT_MAP: &[u8] =
   include_bytes!(concat!(env!("OUT_DIR"), "/CLI_SNAPSHOT.js.map"));
-pub static CLI_SNAPSHOT_DTS: &[u8] =
-  include_bytes!(concat!(env!("OUT_DIR"), "/CLI_SNAPSHOT.d.ts"));
+pub static CLI_SNAPSHOT_DTS: &str =
+  include_str!(concat!(env!("OUT_DIR"), "/CLI_SNAPSHOT.d.ts"));
 
 pub static COMPILER_SNAPSHOT: &[u8] =
   include_bytes!(concat!(env!("OUT_DIR"), "/COMPILER_SNAPSHOT.bin"));
 pub static COMPILER_SNAPSHOT_MAP: &[u8] =
   include_bytes!(concat!(env!("OUT_DIR"), "/COMPILER_SNAPSHOT.js.map"));
-pub static COMPILER_SNAPSHOT_DTS: &[u8] =
-  include_bytes!(concat!(env!("OUT_DIR"), "/COMPILER_SNAPSHOT.d.ts"));
+pub static COMPILER_SNAPSHOT_DTS: &str =
+  include_str!(concat!(env!("OUT_DIR"), "/COMPILER_SNAPSHOT.d.ts"));
 
 #[test]
 fn cli_snapshot() {
-- 
2.22.0

@kitsonk
Copy link
Contributor Author

kitsonk commented Sep 6, 2019

I will take a look at the snapshot that is outputted as it stands... It sounds like an interesting approach, because instead of building it up, like the ts_library_builder did, it would be a "reparse" of the emitted library and a far more lightweight manipulation to just model the few things that can't really be expressed. That should be far more maintainable in the future (and far easier to try to port to Rust eventually).

@ry
Copy link
Member

ry commented Sep 6, 2019

@kitsonk It would be interesting to start with the above patch and see what sort of test errors it generates... My theory is that it's actually not too far from what we need. Maybe reorganizing js/deno.ts js/global.ts js/main.ts etc could allow it to run without a ts_library_builder step?

I haven't got this to work but here's a little experiment I was running to see how rearranging might produce better output: http://tinyclouds.org/tsc_dts_gen.tar.gz

@kitsonk
Copy link
Contributor Author

kitsonk commented Oct 17, 2019

In a conversation with Ryan C at TSConf, I opened up microsoft/TypeScript#34531 which is trying to find a way to emit a single file flattened type definition for a given module. This would allow us to do 90% of the work of the generation of the runtime type library just with the TypeScript compiler.

@kitsonk
Copy link
Contributor Author

kitsonk commented Oct 18, 2019

Actually, what Ryan C and I talked about is actually covered by microsoft/TypeScript#4433. So moving the conversation over to that one.

@kitsonk
Copy link
Contributor Author

kitsonk commented Nov 26, 2019

When this gets implemented we have to make sure to address the workaround done in #3400 and not regress.

@hayd
Copy link
Contributor

hayd commented Nov 26, 2019

We should add a regression test with url as a variable name. (Maybe there already is one, I'm away from my computer...)

@bartlomieju
Copy link
Member

We ended up maintaining declaration files by hand for extended period and now it's the preferable approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants