Prefactorings for introducing transformers #514

tbosch · 2017-06-13T23:14:30Z

No description provided.

evmar

This looks fantastic, thanks a ton for splitting it up! I have only minor comments. I'm not sure if it's worth having someone like Martin also take a look (I think he's on vacation but I don't know when it ends)

evmar · 2017-06-13T23:17:18Z

src/source_map_utils.ts

+
+export const NOOP_SOURCE_MAPPER: SourceMapper = {
+  // tslint:disable-next-line:no-empty
+  addMapping: () => {}


addMapping() {}

?

Sounds good.

evmar · 2017-06-13T23:21:04Z

src/tsickle_compiler_host.ts

+export interface Options extends Es5ProcessorOptions, tsickle.AnnotatorOptions {
+  // This method is here for backwards compatibility.
+  // Use the method in TsickleHost instead.
+  logWarning?: TsickleHost['logWarning'];


I think quoted accesses like this can break optimization. Why not TsickleHost.logWarning?

This is for reusing the type, not the value.

evmar · 2017-06-13T23:28:17Z

src/decorator-annotator.ts

+    }
+  }
+
+  maybeProcessDecorator(node: ts.Node, start?: number): boolean {


Shouldn't this take a ts.Decorator? And then drop the "as ts.Decorator" below?

Sounds good.

evmar · 2017-06-13T23:31:34Z

src/tsickle_compiler_host.ts

@@ -27,7 +27,8 @@ import {isDtsFileName} from './tsickle';
 export enum Pass {
  NONE,
  DECORATOR_DOWNLEVEL,
-  CLOSURIZE
+  CLOSURIZE,
+  DECORATOR_DOWNLEVEL_AND_CLOSURIZE,


Can you add a comment here about how you do both in a single pass, and also that we want to drop the other ones?

Sounds good.

evmar · 2017-06-13T23:35:02Z

src/tsickle_compiler_host.ts

+            sourceFile, this.runConfiguration.oldProgram, fileName, languageVersion, false);
+      case Pass.DECORATOR_DOWNLEVEL_AND_CLOSURIZE:
+        return this.closurize(
+            sourceFile, this.runConfiguration.oldProgram, fileName, languageVersion, true);


Can you stick in some comment like

..., languageVersion, /* also downlevel */ true)

and also above in the false case?

Sounds good.

This is a prefactoring for using transformers.

tbosch · 2017-06-14T04:14:51Z

Attention: This breaks G3, probably a mistake in the enum changes. Need to investigate...

tbosch · 2017-06-14T15:01:23Z

Deleted the commit that changed enums as the following reports the error "Property 'p' of exported interface has or is using private name 'MyEnum'

export interface I {
  p: MyEnum;
}


const MyEnum = {};
type MyEnum = number;
export {MyEnum}

Note that if the variable / type MyEnum is declared before the interface this works!

One part for the annotator itself, and one part for the es5processor. This is a prefactoring for using transformers.

This is a prefactoring for using transformers.

This is a prefactoring for using transformers, as e.g. `.d.ts` files don't run through the transformation pipeline, so we need to generate generate externs for them explicitly.

This is a prefactoring for using transformers, as using multiple Node -> String -> Node transformations would loose too much type information.

This is a prefactoring for using transformers. Typescript always uses double quotes for synthetic string literals.

This is a prefactoring for using transformers, as this is the default way in which typescript emits synthetic code.

These test cases were reproductions of cases where the upcoming transformer version of tsickle failed in G3.

googlebot added the cla: yes label Jun 13, 2017

tbosch mentioned this pull request Jun 13, 2017

Proof of concept: typescript transformer support for tsickle #507

Closed

4 tasks

evmar approved these changes Jun 13, 2017

View reviewed changes

Introduce SourceMapper interface

99bf83e

This is a prefactoring for using transformers.

tbosch force-pushed the prefactor branch 2 times, most recently from c83d1f8 to 7c586b6 Compare June 14, 2017 00:07

tbosch force-pushed the prefactor branch from 7c586b6 to cf99bd8 Compare June 14, 2017 14:59

tbosch force-pushed the prefactor branch from cf99bd8 to 2ae5e69 Compare June 14, 2017 15:40

tbosch added 3 commits June 14, 2017 09:00

Split TsickleHost and Options into parts.

0214eb4

One part for the annotator itself, and one part for the es5processor. This is a prefactoring for using transformers.

Only take a TypeChecker for closure annotating.

566e5d4

This is a prefactoring for using transformers.

Extract writing externs into a separate function

2e932d9

This is a prefactoring for using transformers, as e.g. `.d.ts` files don't run through the transformation pipeline, so we need to generate generate externs for them explicitly.

tbosch force-pushed the prefactor branch from 2ae5e69 to f2defe5 Compare June 14, 2017 16:00

tbosch added 4 commits June 14, 2017 09:25

Allow to lower decorators while annotating closure types

add273a

This is a prefactoring for using transformers, as using multiple Node -> String -> Node transformations would loose too much type information.

Use double quotes for all generated code

a5f10ad

This is a prefactoring for using transformers. Typescript always uses double quotes for synthetic string literals.

Don’t add a line break after null for ctorParameters

4dbf783

This is a prefactoring for using transformers, as this is the default way in which typescript emits synthetic code.

Add more tests cases via goldens for decorators, ambients and module.

0192d97

These test cases were reproductions of cases where the upcoming transformer version of tsickle failed in G3.

tbosch force-pushed the prefactor branch from f2defe5 to 0192d97 Compare June 14, 2017 16:25

tbosch merged commit 5ac90e8 into angular:master Jun 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefactorings for introducing transformers #514

Prefactorings for introducing transformers #514

tbosch commented Jun 13, 2017

evmar left a comment

evmar Jun 13, 2017

tbosch Jun 13, 2017

evmar Jun 13, 2017

tbosch Jun 13, 2017

evmar Jun 13, 2017

tbosch Jun 13, 2017

evmar Jun 13, 2017

tbosch Jun 13, 2017

evmar Jun 13, 2017

tbosch Jun 13, 2017

tbosch commented Jun 14, 2017

tbosch commented Jun 14, 2017

Prefactorings for introducing transformers #514

Prefactorings for introducing transformers #514

Conversation

tbosch commented Jun 13, 2017

evmar 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

tbosch commented Jun 14, 2017

tbosch commented Jun 14, 2017