Next batch of ngtsc changes #24862

alxhub · 2018-07-12T22:25:18Z

No description provided.

mary-poppins · 2018-07-12T22:34:36Z

You can preview 0e78341 at https://pr24862-0e78341.ngbuilds.io/.

mary-poppins · 2018-07-12T22:37:03Z

You can preview 3b182aa at https://pr24862-3b182aa.ngbuilds.io/.

mary-poppins · 2018-07-12T22:52:14Z

You can preview 3dd7c1d at https://pr24862-3dd7c1d.ngbuilds.io/.

JoostK · 2018-07-13T07:14:36Z

packages/compiler-cli/src/ngtsc/annotations/src/directive.ts

@@ -80,17 +80,19 @@ export function extractDirectiveMetadata(
  const decoratedElements =
      members.filter(member => !member.isStatic && member.decorators !== null);

+  const coreModule = isCore && '@angular/core' || undefined;


Write isCore ? '@angular/core' : undefined instead?

JoostK · 2018-07-13T07:15:15Z

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

+    }
+
+    // Construct the list of view queries.
+    const coreModule = this.isCore && '@angular/core' || undefined;


mary-poppins · 2018-07-13T22:44:44Z

You can preview 6fcbc8f at https://pr24862-6fcbc8f.ngbuilds.io/.

mary-poppins · 2018-07-13T22:59:21Z

You can preview 9e3851c at https://pr24862-9e3851c.ngbuilds.io/.

mary-poppins · 2018-07-16T17:01:20Z

You can preview e34dec4 at https://pr24862-e34dec4.ngbuilds.io/.

vicb · 2018-07-17T21:24:59Z

packages/compiler-cli/src/ngtsc/annotations/src/util.ts

@@ -87,3 +87,10 @@ export function referenceToExpression(ref: Reference, context: ts.SourceFile): E
 export function isAngularCore(decorator: Decorator): boolean {
  return decorator.import !== null && decorator.import.from === '@angular/core';
 }
+
+export function unwrapExpression(node: ts.Expression): ts.Expression {


add a one liner comment ?

mary-poppins · 2018-07-17T21:25:47Z

You can preview 504f626 at https://pr24862-504f626.ngbuilds.io/.

vicb · 2018-07-17T21:29:21Z

packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts

+   * Given a `FunctionDeclaration` or `MethodDeclaration`, check if it is typed as a
+   * `ModuleWithProviders` and return an expression referencing the module if available.
+   */
+  private _resolveModuleWithProviders(node: ts.FunctionDeclaration|


IMO the name doesn't reflect what the method does

Renamed to _extractModuleFromModuleWithProvidersFn

vicb · 2018-07-17T21:30:31Z

packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts

-      node, {absoluteModuleName: null, scope: new Map<ts.ParameterDeclaration, ResolvedValue>()});
+export function staticallyResolve(
+    node: ts.Expression, checker: ts.TypeChecker,
+    foreignFunctionResolver?: (node: ts.FunctionDeclaration | ts.MethodDeclaration) =>


pls update the api doc

vicb · 2018-07-17T21:38:00Z

packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts

@@ -356,6 +358,22 @@ class StaticInterpreter {
    return this.visitSymbol(symbol, context);
  }

+  private visitTemplateExpression(node: ts.TemplateExpression, context: Context): ResolvedValue {
+    const pieces: string[] = [node.head.text];
+    for (let i = 0; i < node.templateSpans.length; i++) {


what about tagged template ?
if they are of the same type you should return DYNAMIC_VALUE too.
Add a comment / test ?

Tagged templates are not handled yet, so they'll fall through and return DYNAMIC_VALUE. I don't even know if ngc handles them.

vicb · 2018-07-17T21:54:16Z

packages/compiler-cli/src/ngtsc/annotations/src/directive.ts

+    predicate = new WrappedNodeExpr(args[0]);
+  } else if (typeof arg === 'string') {
+    predicate = [arg];
+  } else if (assertIsStringArray(arg, '@' + name)) {


assert are often debug time checks only, rename/refactor ?

Renamed to isStringArrayOrDie()

vicb · 2018-07-17T21:55:00Z

packages/compiler-cli/src/ngtsc/annotations/src/directive.ts

+  } else if (assertIsStringArray(arg, '@' + name)) {
+    predicate = arg as string[];
+  }
+  if (predicate === null) {


should it be a final else {} ?

vicb · 2018-07-17T21:56:34Z

packages/compiler-cli/src/ngtsc/annotations/src/directive.ts

+    }
+
+    if (options.has('descendants')) {
+      const descendantsValue = staticallyResolve(options.get('descendants') !, checker);


nit: remove the local variable and use descendants ?

The assignment isn't legal until after the typeof check.

vicb · 2018-07-17T21:57:37Z

packages/compiler-cli/src/ngtsc/annotations/src/directive.ts

+
+  // Extract the read and descendants options.
+  let read: Expression|null = null;
+  let descendants: boolean = true;


shouldn't descendants be a function of first ? (I think the behavior is different, can't remember exactly)

It's part of the options argument, as far as I know. first means whether it's ViewChild or ViewChildren.

See

angular/packages/core/src/metadata/di.ts

Line 192 in 0b28732

({selector, first: false, isViewQuery: false, descendants: false, ...data}),

ContentChild and ContentChildren have a different default (ViewChild and ViewChildren both default to true)

vicb · 2018-07-17T22:02:50Z

packages/compiler-cli/src/ngtsc/annotations/src/directive.ts

+            if (decorator.args.length === 2) {
+              const resolvedArgs = staticallyResolve(decorator.args[1], checker);
+              if (!assertIsStringArray(resolvedArgs, '@HostListener.args')) {
+                throw new Error(`@HostListener arguments must be a string array`);


nit: arguments -> second argument would be clearer IMO

vicb · 2018-07-17T22:04:11Z

packages/compiler-cli/test/ngtsc/ngtsc_spec.ts

+        })
+        class FooCmp {
+          @ContentChild('bar', {read: TemplateRef}) child: any;
+          @ContentChildren(TemplateRef) children: any;


Add tests for @View... and getters / setters ?

vicb · 2018-07-17T22:04:40Z

packages/compiler-cli/test/ngtsc/ngtsc_spec.ts

+          template: 'Test',
+          host: {
+            '[attr.hello]': 'foo',
+            '(click)': 'onClick($event)',


add test for args ?

vicb · 2018-07-17T22:27:19Z

packages/compiler-cli/src/ngtsc/annotations/src/directive.ts

@@ -128,7 +128,7 @@ export function extractDirectiveMetadata(

  // Determine if `ngOnChanges` is a lifecycle hook defined on the component.
  const usesOnChanges = members.find(


use members.some() instead

vicb · 2018-07-17T22:31:05Z

packages/compiler-cli/src/ngtsc/transform/src/translator.ts

+export class Context {
+  constructor(readonly isStatement: boolean) {}
+
+  get setExpressionMode(): Context { return this.isStatement ? new Context(false) : this; }


nit: weird that a getter as side effect. make it a regular method ?

Renamed to withExpressionMode / withStatementMode.

The getters are pure and do not have side effects (they do not modify the current context class). They are, however, lazy - a new Context is only created if it would be a change from the current Context.

This commit adds the ivy-local tag to //packages/router. Since the router depends on //packages/upgrade, it makes that package compatible with ngtsc as well. PR Close #24862

@NgModule

Metadata in Ivy must be literal. For example, @NgModule({...}) is legal, whereas const meta = {...}; @NgModule(meta) is not. However, some code contains additional superfluous parentheses: @NgModule(({...})) It is desirable that ngtsc accept this form of literal object. PR Close #24862

@NgModule

…4862) Within an @NgModule it's common to include in the imports a call to a ModuleWithProviders function, for example RouterModule.forRoot(). The old ngc compiler was able to handle this pattern because it had global knowledge of metadata of not only the input compilation unit but also all dependencies. The ngtsc compiler for Ivy doesn't have this knowledge, so the pattern of ModuleWithProviders functions is more difficult. ngtsc must be able to determine which module is imported via the function in order to expand the selector scope and properly tree-shake directives and pipes. This commit implements a solution to this problem, by adding a type parameter to ModuleWithProviders through which the actual module type can be passed between compilation units. The provider side isn't a problem because the imports are always copied directly to the ngInjectorDef. PR Close #24862

PR Close #24862

This commit adds support for template substitution expressions for ngtsc static resolution. PR Close #24862

PR Close #24862

Previously ngtsc had a few bugs handling special token types: * Injector was not properly translated to INJECTOR * ChangeDetectorRef was not injected via injectChangeDetectorRef() This commit fixes these two bugs, and also adds a test to ensure they continue to work correctly. PR Close #24862

@NgModule

@NgModule()s get compiled to two fields: ngModuleDef and ngInjectorDef. Both fields contain imports, as both selector scopes and injectors have the concept of composed units of configuration. Previously these fields were generated by static resolution of imports and exports in metadata. Support for ModuleWithProviders requires they be generated differently. ngModuleDef's imports/exports are generated as resolved lists of types, whereas ngInjectorDef's imports should reflect the raw expressions that the developer wrote in the metadata. This change modifies the NgModule handler and properly copies raw nodes for the imports and exports into the ngInjectorDef. PR Close #24862

PR Close #24862

@NgModule

ngInjectorDef.imports is generated from @NgModule.imports plus @NgModule.exports. A problem arises as a result, because @NgModule exports contain not only other modules (which will have ngInjectorDef fields), but components, directives, and pipes as well. Because of locality, it's difficult for the compiler to filter these out at build time. It's not impossible, but for now filtering them out at runtime will allow testing of the compiler against complex applications. PR Close #24862

PR Close #24862

@ContentChild

This commit adds support for @ContentChild[ren] and @ViewChild[ren] in ngtsc. Previously queries were ignored. PR Close #24862

This change adds support for host bindings to ngtsc, and parses them both from decorators and from the metadata in the top-level annotation. PR Close #24862

Previously, the static resolver did its own interpretation of statements in the TypeScript AST, which only functioned on TypeScript code. ES5 code in particular would not work with the resolver as it had hard-coded assumptions about AST structure. This commit changes the resolver to use a ReflectionHost instead, which abstracts away understanding of the structural side of the AST. It adds 3 new methods to the ReflectionHost in support of this functionality: * getDeclarationOfIdentifier * getExportsOfModule * isClass PR Close #24862

Previously ngtsc had a bug where it would only detect the presence of ngOnChanges as a static method. This commit flips the condition and only recognizes ngOnChanges as a non-static method. PR Close #24862

…24862) Previously, references had the concept of an identifier, but would not properly detect whether the identifier should be used or not when generating an expression. This change fixes that logic. Additionally, now whenever an identifier resolves to a reference (even one imported from another module) as part of resolving an expression, the reference is updated to use that identifier. This ensures that for a class Foo declared in foo.ts, but referenced in an expression in bar.ts, the Reference returned includes the identifier from bar.ts, meaning that writing an expression in bar.ts for the Reference will not generate an import. PR Close #24862

…24862) Previously, when translating an assignment expression (e.g. x = 3), the translator would always print the statement as X = Y. However, if the expression is included in a larger expression (X = (Y = Z)), the translator would print "X = Y = Z" without regard for the outer expression context. Now, the translator understands when it's printing an expression statement (X = Y;) vs an expression in a larger context (X = (Y = Z);) and encapsulates the latter in parentheses. PR Close #24862

This commit moves the compiler compliance tests into compiler-cli, and uses ngtsc to run them instead of the custom compilation pipeline used before. Testing against ngtsc allows for validation of the real compiler output. This commit also fixes a few small issues that prevented the tests from passing. PR Close #24862

Previously, some of the *Def symbols were not exported or were exported as public API. This commit ensures every definition type is in the private export namespace. PR Close #24862

@NgModule

Previously ngtsc would use a tuple of class types for listing metadata in .d.ts files. For example, an @NgModule's declarations might be represented with the type: [NgIf, NgForOf, NgClass] If the module had no declarations, an empty tuple [] would be produced. This has two problems. 1. If the class type has generic type parameters, TypeScript will complain that they're not provided. 2. The empty tuple type is not actually legal. This commit addresses both problems. 1. Class types are now represented using the `typeof` operator, so the above declarations would be represented as: [typeof NgIf, typeof NgForOf, typeof NgClass]. Since typeof operates on a value, it doesn't require generic type arguments. 2. Instead of an empty tuple, `never` is used to indicate no metadata. PR Close #24862

PR Close #24862

Ivy definition types have a generic type which specifies the return type of the factory function. For example: static ngDirectiveDef<NgForOf, '[ngFor][ngForOf]'> However, in this case NgForOf itself has a type parameter <T>. Thus, writing the above is incorrect. This commit modifies ngtsc to understand the genericness of NgForOf and to write the following: static ngDirectiveDef<NgForOf<any>, '[ngFor][ngForOf]'> PR Close #24862

angular-automatic-lock-bot · 2019-09-13T20:22:39Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Jul 12, 2018

alxhub force-pushed the ngtsc-integration branch from 0e78341 to 3b182aa Compare July 12, 2018 22:29

alxhub requested a review from mhevery July 12, 2018 22:30

alxhub force-pushed the ngtsc-integration branch from 3b182aa to 3dd7c1d Compare July 12, 2018 22:43

mhevery added the area: core Issues related to the framework runtime label Jul 12, 2018

JoostK reviewed Jul 13, 2018

View reviewed changes

alxhub force-pushed the ngtsc-integration branch from 6fcbc8f to 9e3851c Compare July 13, 2018 22:50

alxhub force-pushed the ngtsc-integration branch from 9e3851c to e34dec4 Compare July 16, 2018 16:49

alxhub force-pushed the ngtsc-integration branch from e34dec4 to 504f626 Compare July 17, 2018 21:16

vicb reviewed Jul 17, 2018

View reviewed changes

alxhub force-pushed the ngtsc-integration branch from 504f626 to ffec1f1 Compare July 17, 2018 22:32

alxhub added target: major This PR is targeted for the next major release action: merge The PR is ready for merge by the caretaker labels Jul 20, 2018

mhevery approved these changes Jul 20, 2018

View reviewed changes

vicb closed this in f58f3dc Jul 20, 2018

vicb pushed a commit that referenced this pull request Jul 20, 2018

refactor(ivy): fix an unnecessarily deep import (#24862)

e346c3c

PR Close #24862

vicb pushed a commit that referenced this pull request Jul 20, 2018

feat(ivy): statically resolve template expressions (#24862)

8a986d4

This commit adds support for template substitution expressions for ngtsc static resolution. PR Close #24862

vicb pushed a commit that referenced this pull request Jul 20, 2018

fix(ivy): export InheritDefinitionFeature (#24862)

53a1600

PR Close #24862

vicb pushed a commit that referenced this pull request Jul 20, 2018

fix(ivy): export NgModuleFactory adapter (#24862)

ae45632

PR Close #24862

vicb pushed a commit that referenced this pull request Jul 20, 2018

fix(ivy): fix a couple issues with Input/Output compilation (#24862)

6eb6ac7

PR Close #24862

vicb pushed a commit that referenced this pull request Jul 20, 2018

feat(ivy): compile queries in ngtsc (#24862)

76f8f78

This commit adds support for @ContentChild[ren] and @ViewChild[ren] in ngtsc. Previously queries were ignored. PR Close #24862

vicb pushed a commit that referenced this pull request Jul 20, 2018

feat(ivy): support host bindings in ngtsc (#24862)

2e724ec

This change adds support for host bindings to ngtsc, and parses them both from decorators and from the metadata in the top-level annotation. PR Close #24862

vicb pushed a commit that referenced this pull request Jul 20, 2018

fix(ivy): export injectElementRef (#24862)

2b8b647

PR Close #24862

petebacondarwin mentioned this pull request Jul 22, 2018

feat(ivy): port the static resolver to use the ReflectionHost #24860

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

		@@ -128,7 +128,7 @@ export function extractDirectiveMetadata(

		// Determine if `ngOnChanges` is a lifecycle hook defined on the component.
		const usesOnChanges = members.find(

Next batch of ngtsc changes #24862

Next batch of ngtsc changes #24862

Conversation

alxhub commented Jul 12, 2018

mary-poppins commented Jul 12, 2018

mary-poppins commented Jul 12, 2018

mary-poppins commented Jul 12, 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

mary-poppins commented Jul 13, 2018

mary-poppins commented Jul 13, 2018

mary-poppins commented Jul 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Jul 17, 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

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

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

angular-automatic-lock-bot bot commented Sep 13, 2019