Add support for --types #960

kitsonk · 2018-10-11T08:18:02Z

Resolve #632

This is a continuation of previous pull requests (#952 and #729) which addresses additional feedback and includes basic unit tests for the ts_library_builder.

kitsonk · 2018-10-11T09:39:39Z

@ry ok, it appears to be finally ready to go, if you are happy with the unit tests (and there is further review by @piscisaureus)

cc: @dsherret

dsherret · 2018-10-11T16:00:16Z

tools/ts_library_builder/build_library.ts

+  for (const enumName of enumNames) {
+    const enumDeclaration = sourceFile.getEnumOrThrow(enumName);
+    enumDeclaration.setHasDeclareKeyword(false);
+    output += enumDeclaration.getText();


You probably already know this and not sure if it's necessary here, but just take note that .getText() does not include the js docs. If you want that, there is a .getText(true) argument that will include the js docs when getting the text (not a fan of obscure boolean parameters, but this patterns off getStart(includeJsDocComments = false) from the compiler api).

Also note, .getText() returns the text without leading trivia (comments & whitespace) while .getFullText() returns the text with leading trivia.

Specifically here the generated declaration file includes a useless generated JSDoc comment so it is intentional, but I should comment that.

ry · 2018-10-11T16:42:04Z

tools/ts_library_builder/main_test.ts

+//  ./node_modules/.bin/ts-node --project tools/ts_library_builder/tsconfig.json tools/ts_library_builder/main_test.ts
+
+import "./ast_util_test";
+import "./build_library_test";


Rename main_test.ts to test.ts

This isn't hooked into tools/test.py yet. Add the following to tools/unit_tests.py, and check that tools/test.py fails if you change an assert inside one of your tests:

run([ "node", "./node_modules/.bin/ts-node", "--project", "tools/ts_library_builder/tsconfig.json", "tools/ts_library_builder/test.ts" ])

FYI I'd like to land #958 before this, so that we can also get the benefit of output checking.

ry · 2018-10-11T16:43:19Z

tools/ts_library_builder/build_library_test.ts

+    variableDeclarations[2].getType().getText(),
+    `typeof import("bazqat").moduleC.qat`
+  );
+  assertEqual(variableDeclarations.length, 3);


Great. Thanks!

Move these into test.ts

ry · 2018-10-11T16:44:10Z

tools/ts_library_builder/ast_util_test.ts

+test(function astUtilNormalizeSlashes() {
+  // TODO build out
+  assert(!!normalizeSlashes);
+});


No need to add all these stubs - it's just more to maintain. Just put all the tests into test.ts.

piscisaureus · 2018-10-11T20:27:57Z

@kitsonk Do you have a grip on how hard it would be to use the TypeScript AST for this rather than the simple-ast wrapper?
(this is not blocking the patch from landing btw)

piscisaureus · 2018-10-11T20:33:50Z

tools/ts_library_builder/ast_util.ts

+  });
+}
+
+/** Add `@source` comment to node. */


Does a URL make more sense?
I think we're mostly doing this for our imaginative users who don't have the source code checked out.

Well, I was doing it to be able to see where it came from when chasing down issues... I think also in the future, if an interface or API pops into the generated library and we want to not "leak" it and want to mark it with @internal, being able to determine that in debug mode would make it easier. @url can make more sense/is a more standard JSDoc pragma.

piscisaureus · 2018-10-11T20:38:15Z

src/flags.rs

@@ -58,7 +59,8 @@ pub fn print_usage() {
 -D or --log-debug  Log debug output.
 -h or --help       Print this message.
 --v8-options       Print V8 command line options.
--deps             Print module dependencies."
+--deps             Print module dependencies.
+--types            Print global environment types."


Maybe better: print typescript declarations for the deno standard library.
cc @ry

"Print runtime typescript declarations."

Print the type declarations for the runtime environment.?

Crossed the streams... I will go with what @ry suggested.

piscisaureus

I'd suggest to add find_exts("tools/ts_library_builder", ".js", ".json", ".ts", ".md") to tools/format.py so all these files get formatted.

kitsonk · 2018-10-11T20:53:21Z

Sorry @piscisaureus I didn't see your feedback before doing my latest push. I agree with all of it and will address it.

As far as the native TypeScript AST, I have always found it a bit obtuse. It isn't anything like any of the ES ASTs that I have used before, it is largely based on Roselyn (so C#-esque) which makes sense to Microsoft compiler folks but not to me personally. The ts-simple-ast is a far more functional abstraction in my opinion and while it isn't really like the ES AST parsers, it certainly is very usable. If someone was more used to Roselyn, then I am sure it would make a lot more sense, but that person is not me.

piscisaureus

LGTM. This is great!
@ry land when you think it's ready

ry · 2018-10-11T21:41:29Z

@kitsonk Still missing integration into tools/unit_tests.py

ry · 2018-10-11T21:47:48Z

@kitsonk rebase to master and apply this patch (but squash it into your commits)

From d8c0913984e4b97b133bf85ba5b4e9d3c639249d Mon Sep 17 00:00:00 2001
From: Ryan Dahl <ry@tinyclouds.org>
Date: Thu, 11 Oct 2018 17:47:14 -0400
Subject: [PATCH] Add tools/ts_library_builder/test.ts to unit_tests/py

---
 tools/unit_tests.py | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/tools/unit_tests.py b/tools/unit_tests.py
index b9be484..14f1d18 100755
--- a/tools/unit_tests.py
+++ b/tools/unit_tests.py
@@ -4,9 +4,7 @@ import sys
 import subprocess
 import re
 
-
-def run_unit_test(deno_exe, permStr, flags=[]):
-    cmd = [deno_exe, "--reload", "js/unit_tests.ts", permStr] + flags
+def run_unit_test2(cmd):
     process = subprocess.Popen(
         cmd,
         bufsize=1,
@@ -28,6 +26,10 @@ def run_unit_test(deno_exe, permStr, flags=[]):
     if errcode != 0:
         sys.exit(errcode)
 
+def run_unit_test(deno_exe, permStr, flags=[]):
+    cmd = [deno_exe, "--reload", "js/unit_tests.ts", permStr] + flags
+    run_unit_test2(cmd)
+
 
 # We want to test many ops in deno which have different behavior depending on
 # the permissions set. These tests can specify which permissions they expect,
@@ -43,6 +45,16 @@ def unit_tests(deno_exe):
     # TODO We might accidentally miss some. We should be smarter about which we
     # run. Maybe we can use the "filtered out" number to check this.
 
+    # These are not strictly unit tests for Deno, but for ts_library_builder.
+    # They run under Node, but use the same //js/testing/ library.
+    run_unit_test2([
+        "node",
+        "./node_modules/.bin/ts-node",
+        "--project",
+        "tools/ts_library_builder/tsconfig.json",
+        "tools/ts_library_builder/test.ts"
+    ])
+
 
 if __name__ == '__main__':
     if len(sys.argv) < 2:
-- 
2.15.0

ry · 2018-10-11T22:17:18Z

Landing with above patch in #967

ry requested a review from piscisaureus October 11, 2018 15:36

dsherret reviewed Oct 11, 2018

View reviewed changes

ry reviewed Oct 11, 2018

View reviewed changes

piscisaureus reviewed Oct 11, 2018

View reviewed changes

kitsonk force-pushed the types2 branch from 51649ca to 1f97989 Compare October 11, 2018 20:45

piscisaureus approved these changes Oct 11, 2018

View reviewed changes

kitsonk force-pushed the types2 branch from 1f97989 to e046525 Compare October 11, 2018 21:12

kitsonk added 2 commits October 12, 2018 08:23

Replace globals.d.ts with lib.deno_runtime.d.ts

c3639b7

Add support for --types

d21287b

kitsonk force-pushed the types2 branch from e046525 to d21287b Compare October 11, 2018 21:23

ry closed this Oct 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for --types #960

Add support for --types #960

kitsonk commented Oct 11, 2018

kitsonk commented Oct 11, 2018

dsherret Oct 11, 2018

kitsonk Oct 11, 2018

ry Oct 11, 2018 •

edited

Loading

ry Oct 11, 2018 •

edited

Loading

ry Oct 11, 2018 •

edited

Loading

ry Oct 11, 2018

piscisaureus commented Oct 11, 2018

piscisaureus Oct 11, 2018

kitsonk Oct 11, 2018

piscisaureus Oct 11, 2018

ry Oct 11, 2018

kitsonk Oct 11, 2018

kitsonk Oct 11, 2018

piscisaureus left a comment

kitsonk commented Oct 11, 2018

piscisaureus left a comment •

edited

Loading

ry commented Oct 11, 2018

ry commented Oct 11, 2018 •

edited

Loading

ry commented Oct 11, 2018

Add support for --types #960

Add support for --types #960

Conversation

kitsonk commented Oct 11, 2018

kitsonk commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

ry Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

ry Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piscisaureus commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piscisaureus left a comment

Choose a reason for hiding this comment

kitsonk commented Oct 11, 2018

piscisaureus left a comment • edited Loading

Choose a reason for hiding this comment

ry commented Oct 11, 2018

ry commented Oct 11, 2018 • edited Loading

ry commented Oct 11, 2018

ry Oct 11, 2018 •

edited

Loading

ry Oct 11, 2018 •

edited

Loading

ry Oct 11, 2018 •

edited

Loading

piscisaureus left a comment •

edited

Loading

ry commented Oct 11, 2018 •

edited

Loading