Move from `runtime.ts` to `compiler.ts` #518

kitsonk · 2018-08-14T20:05:56Z

Resolves #509

Migrates from runtime.ts to compiler.ts, providing a better abstraction for compiling modules within deno. It also includes a significant amount of unit test coverage for the compiler.

ry

Thanks for working thru this. First pass comments below-

ry · 2018-08-17T15:58:16Z

js/compiler.ts

+  interface Window {
+    define?: AmdDefine;
+  }
+}


These "declare global" things confuse me. Is this meant to remain here or do you intend for this to move to globals.ts? If the latter please add a TODO.

They are intended to be global augmentations, and live in the module where the code that impacts the global scope lives, so they don't get disconnected. For example if you had something like jQuery which created a $ variable in the global scope, you would want to have that definition in the jQuery module.

Upon reflection, it is unnecessary in this case, unless we wanted to ensure we ensure that end user code does not attempt to touch it, in which case it should go into the global.d.ts as a readonly property.

ry · 2018-08-17T16:00:23Z

js/compiler.ts

+ * A simple object structure for caching resolved modules and their contents.
+ *
+ * Named `FileModule` to clarify it is just a representation of meta data of
+ * the module, not the actual module instance.


Maybe we should call it ModuleMetaData?

ry · 2018-08-17T16:01:23Z

js/compiler.ts

+ * The required minimal API to allow formatting of TypeScript compiler
+ * diagnostics.
+ */
+const formatDiagnosticsHost = {


Is there an interface that this implements? If so it would be good to add a type here.

ry · 2018-08-17T16:01:58Z

js/compiler.ts

+ */
+export class DenoCompiler implements ts.LanguageServiceHost {
+  private readonly _fileModuleMap = new Map<string, FileModule>();
+  private readonly _fileNamesMap = new Map<string, Map<string, string>>();


nit: why the underscores?

_fileNamesMap isn't clear from the type definition what it does - maybe add a comment - or better do something like:

type ModuleSpecifier = string type Foo = string type Bar = string private readonly _fileNamesMap = new Map<ModuleSpecifier, Map<Foo, Bar>>();

the underscore, convention... runtime.ts uses the convention inconsistently:

deno/js/runtime.ts

Line 248 in 81b042c

private static _instance: Compiler;

and

deno/js/runtime.ts

Line 241 in 81b042c

private service: ts.LanguageService;

I have always prefaced private members with _ for as long as I can remember, back in pre-TS days. The only real argument is that it makes it abundantly clear, even at access, that you are accessing something that is private. It also ensures your public surface is less likely to run into odd conflicts. One potential other argument is that ES private members will have the # sigil, so a future conversion to adopt this would be a "simple" find and replace.

The type aliases will make things clearer, I like that. 👍

ry · 2018-08-17T16:05:42Z

js/compiler.ts

+  private readonly _fileNamesMap = new Map<string, Map<string, string>>();
+
+  // TODO ideally this are not static and can be influenced by command line
+  // arguments


Link to #51

ry · 2018-08-17T16:23:05Z

js/compiler.ts

+
+  readFile(path: string): string | undefined {
+    log("readFile", path);
+    throw Error("not implemented");


// TODO #534

ry · 2018-08-17T16:25:39Z

js/compiler_tests.ts

+        outputCode: string
+    ): void {},
+    codeFetch(moduleSpecifier: string, containingFile: string): ModuleInfo {
+        return {


Add a counter to the mocked methods so we can check how many times they were called.

ry · 2018-08-17T16:26:22Z

js/compiler_tests.ts

+
+test(function compiler_tests_teardown() {
+    Object.assign(compiler, originals);
+});


Good start.

ry · 2018-08-17T16:28:59Z

tests/error_003.ts

@@ -0,0 +1 @@
+consol.log("hello world!");


Add a comment like

// Note console.log is intentionally misspelled: consol.log("hello world!");

ry · 2018-08-17T16:29:28Z

tests/error_003.ts.out

+[30;47m1[0m consol.log("hello world!");
+[30;47m [0m [91m~~~~~~[0m
+
+  [96m$asset$/globals.d.ts[WILDCARD]


I'm not sure if I like all the term escapes here... but it's fine for now.

At the moment, the matching will fail, because they are output with colours. So I don't know if it is make the matching strip/ignore them or put them in the .out.

ry · 2018-08-17T17:08:09Z

js/compiler_tests.ts

+  assert(DenoCompiler.instance() != null);
+});
+
+test(function compiler_tests_setup() {


Please continue to use camelCase for tests. Prefix all compiler test names with compiler.

(I think unit_tests isn't doing this properly)

It isn't... it was 🐵👁🐵do on my part. 🤣

ry

Looks good!
No need to test every method (there is a danger in being too prescriptive early on in a project). The main thing I want to see is how the compiler interacts with the rust system- via call counts to codeFetch/codeCache. I guess you're getting there still - since there's no test for the compile method yet?

ry · 2018-08-19T22:27:32Z

js/compiler_tests.ts

+  assert(compilerInstance.getScriptKind("foo.d.ts") === 3);
+  assert(compilerInstance.getScriptKind("foo.js") === 1);
+  assert(compilerInstance.getScriptKind("foo.json") === 6);
+  assert(compilerInstance.getScriptKind("foo.txt") === 1);


Why not use compiler.ScriptKind.JS and friends ?

So there is an issue, for some reason, they don't exist at runtime. I had to create them in compiler.ts, I guess I need to pull them from there too.

ry · 2018-08-19T22:35:46Z

js/compiler_tests.ts

+test(function compilerRun() {
+  const result = compilerInstance.run("foo/bar.ts", "/root/project");
+  assert(result != null);
+});


Would be good to check how many times codeFetch and codeCache are called here.

👍 Yeah, this was just a basic "hack" at this point... going to introspect everything, and try to get full coverage on that, ensuring we get the behaviour we expect.

kitsonk · 2018-08-20T03:06:04Z

@ry, thanks for the feedback. I get the point about "too many tests" but getting full coverage on the TypeScript APIs seemed to be logical for me, as that is obviously a fixed API on the TypeScript side, and somewhat immutable (and also really important for accepting upgrades of TypeScript).

kitsonk · 2018-08-21T18:14:38Z

@ry, no longer WIP and ready for a final (maybe?) review

ry

Looks good. I have a few comments in the tests.

It seems like this would be best done in a couple commits rather than squashing it all into one? Can you organize/squash the commits into something we can land without further modification?

ry · 2018-08-21T18:47:23Z

js/compiler_tests.ts

+  // equal to `deno foo/bar.ts`
+  const codeFetchLength = codeFetchStack.length;
+  const codeCacheLength = codeCacheStack.length;
+  const globalEvalLength = globalEvalStack.length;


Awesome work on the tests. Why not just assert that these are empty here, rather than recording the length? Since you're always popping off everything, they should be empty.

ry · 2018-08-21T18:48:59Z

js/compiler_tests.ts

+  ]) {
+    assert(key in result, `Expected "${key}" in compiler options.`);
+  }
+  assertEqual(Object.keys(result).length, 7, "Expected only 7 keys.");


This assert seems excessively specific to me.

ry · 2018-08-21T18:50:36Z

js/compiler_tests.ts

+  );
+  const result = compilerInstance.getScriptSnapshot(moduleMetaData.fileName);
+  assert(result != null, "Expected snapshot to be defined.");
+  assertEqual(result.getLength(), 87, "Expected a length of 87.");


Instead of using 87, use fooBarTsSource.length. No need for excessively verbose assert messages either.

ry · 2018-08-21T18:52:22Z

js/compiler_tests.ts

+    "Expected .getText() to equal 'import'"
+  );
+  assertEqual(result.getChangeRange(result), undefined);
+  assert(!("dispose" in result));


Why "dispose" ? Maybe add a comment?

ry · 2018-08-21T18:54:00Z

tests/error_004_missing_module.ts.out

@@ -0,0 +1,12 @@
+Error: Cannot resolve module "bad-module.ts" from "[WILDCARD]error_004_missing_module.ts".


Good! thanks!

ry · 2018-08-22T13:49:36Z

js/compiler_tests.ts

@@ -0,0 +1,466 @@
+// Copyright 2018 the Deno authors. All rights reserved. MIT license.


Rename this file to js/compiler_test.ts

ry · 2018-08-22T13:52:28Z

js/compiler_tests.ts

+    exports.foo = "bar";
+});
+//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoiYmFyLmpzIiwic291cmNlUm9vdCI6IiIsInNvdXJjZXMiOlsiZmlsZTovLy9yb290L3Byb2plY3QvZm9vL2Jhci50cyJdLCJuYW1lcyI6W10sIm1hcHBpbmdzIjoiOzs7SUFDQSxPQUFPLENBQUMsR0FBRyxDQUFDLFFBQVEsQ0FBQyxDQUFDO0lBQ1QsUUFBQSxHQUFHLEdBQUcsS0FBSyxDQUFDIiwic291cmNlc0NvbnRlbnQiOlsiaW1wb3J0ICogYXMgY29tcGlsZXIgZnJvbSBcImNvbXBpbGVyXCI7XG5jb25zb2xlLmxvZyhjb21waWxlcik7XG5leHBvcnQgY29uc3QgZm9vID0gXCJiYXJcIjtcbiJdfQ==
+//# sourceURL=/root/project/foo/bar.ts`;


If possible, it would be nice to not have the source map stuff in here. Maybe that needs to wait until we disable the inlineSourceMaps - if so please add TODO

// TODO Remove source map strings from fooBarTsOutput. // https://github.com/denoland/deno/issues/23

ry · 2018-08-22T14:01:55Z

js/compiler.ts

+
+/**
+ * Abstraction of the APIs required from the `os` module so they can be
+ * easily substituted.


s/substituted/mocked/

ry · 2018-08-22T14:02:01Z

js/compiler.ts

+
+/**
+ * Abstraction of the APIs required from the `typescript` module so they can
+ * be easily substituted.


s/substituted/mocked/

ry · 2018-08-22T14:12:06Z

js/compiler.ts

+type ContainingFile = string;
+type ModuleFileName = string;
+type ModuleSpecifier = string;
+type OutputCode = string;


Add some comments here:

// The location that a module is being loaded from. This could be a directory, like ".", or it could be a module specifier like "http://gist.github.com/somefile.ts" type ContainingFile = string; // The internal local filename of a compiled module. It will often be something like "/home/ry/.deno/gen/f7b4605dfbc4d3bb356e98fda6ceb1481e4a8df5.js" type ModuleFileName = string; // The external name of a module - could be a URL or could be a relative path. Examples "http://gist.github.com/somefile.ts" or "./somefile.ts" type ModuleSpecifier = string; // The compiled source code which is cached in .deno/gen/ type OutputCode = string;

ry · 2018-08-22T14:37:35Z

js/compiler.ts

+          return DenoCompiler._builtins[dep];
+        } else {
+          const depModuleMetaData = this.run(dep, moduleMetaData.fileName);
+          return depModuleMetaData.exports;


This code is duplicated just above in the try-catch? I'm having trouble following the logic here. Is this a programming error? If not, why are we mapping the deps twice?

Not done. @kitsonk please take a look at this when you get a chance.

ry · 2018-08-22T20:13:50Z

Continuing in #564

typescript-eslint/typescript-eslint#637

kitsonk force-pushed the ts_runtime_improve branch from 417497b to 3b36f29 Compare August 16, 2018 18:04

kitsonk changed the title ~~[WIP] Improve runtime.ts~~ [WIP] Move from runtime.ts to compiler.ts Aug 16, 2018

ry reviewed Aug 17, 2018

View reviewed changes

kitsonk force-pushed the ts_runtime_improve branch from 8a6d435 to ae6671b Compare August 17, 2018 16:50

ry reviewed Aug 17, 2018

View reviewed changes

kitsonk mentioned this pull request Aug 18, 2018

Better error message when bad filename is loaded #526

Closed

kitsonk force-pushed the ts_runtime_improve branch 2 times, most recently from c823e37 to 8f1fd8d Compare August 19, 2018 21:16

ry reviewed Aug 19, 2018

View reviewed changes

kitsonk added 2 commits August 21, 2018 11:04

Migrate from runtime.ts to compiler.ts

45d8138

Add additional tests related to compiler

7bb5ec6

kitsonk force-pushed the ts_runtime_improve branch from 158c9e0 to 7bb5ec6 Compare August 21, 2018 18:08

kitsonk changed the title ~~[WIP] Move from runtime.ts to compiler.ts~~ Move from runtime.ts to compiler.ts Aug 21, 2018

ry reviewed Aug 21, 2018

View reviewed changes

ry reviewed Aug 22, 2018

View reviewed changes

ry mentioned this pull request Aug 22, 2018

window.onerror should be libdeno.setGlobalErrorHandler() #553

Closed

ry reviewed Aug 22, 2018

View reviewed changes

This was referenced Aug 22, 2018

HTTP imports #2 #530

Merged

runtime.ts refactor into compiler.ts #564

Merged

ry closed this Aug 22, 2018

piscisaureus pushed a commit to piscisaureus/deno that referenced this pull request Oct 7, 2019

fix: pin eslint version for CI (denoland#518)

4772c34

typescript-eslint/typescript-eslint#637

kitsonk deleted the ts_runtime_improve branch August 2, 2022 04:41

		@@ -0,0 +1,12 @@
		Error: Cannot resolve module "bad-module.ts" from "[WILDCARD]error_004_missing_module.ts".

		@@ -0,0 +1,466 @@
		// Copyright 2018 the Deno authors. All rights reserved. MIT license.

Move from runtime.ts to compiler.ts #518

Move from runtime.ts to compiler.ts #518

Conversation

kitsonk commented Aug 14, 2018 • edited Loading

ry left a 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

Choose a reason for hiding this comment

kitsonk Aug 18, 2018 • edited Loading

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

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

ry left a 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

kitsonk commented Aug 20, 2018

kitsonk commented Aug 21, 2018

ry left a 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

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

ry Aug 22, 2018 • edited Loading

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

ry Aug 22, 2018 • edited Loading

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

ry commented Aug 22, 2018

Move from `runtime.ts` to `compiler.ts` #518

Move from `runtime.ts` to `compiler.ts` #518

kitsonk commented Aug 14, 2018 •

edited

Loading

kitsonk Aug 18, 2018 •

edited

Loading

ry Aug 22, 2018 •

edited

Loading

ry Aug 22, 2018 •

edited

Loading