Batch of ngtsc changes for Material, CLI, g3 compatibility. #25080

alxhub · 2018-07-25T00:15:49Z

No description provided.

mary-poppins · 2018-07-25T00:26:08Z

You can preview 87f72d7 at https://pr25080-87f72d7.ngbuilds.io/.

gkalpak · 2018-07-25T09:12:52Z

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

-  foreignFunctionResolver?(node: ts.FunctionDeclaration|ts.MethodDeclaration): ts.Expression|null;
+  foreignFunctionResolver?
+      (ref: Reference<ts.FunctionDeclaration|ts.MethodDeclaration>,
+       args: ReadonlyArray<ts.Expression>): ts.Expression|null;


Why a ReadonlyArray here but not in other places?

So it can directly accept the .arguments property of a ts.CallExpression, which is a NodeArray that extends ReadonlyArray.

gkalpak · 2018-07-25T09:15:56Z

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

+    return node;
+  }
+  const arg = node.arguments[0];
+  if (!ts.isArrowFunction(arg) || ts.isBlock(arg.body)) {


Why does it have to be an arrow function?
Wouldn't that be a problem for (f)esm5 in ngcc?

Good point. Fixed.

vicb · 2018-07-25T17:18:35Z

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

@@ -14,7 +14,7 @@ import {Reference, filterToMembersWithDecorator, reflectObjectLiteral, staticall
 import {AnalysisOutput, CompileResult, DecoratorHandler} from '../../transform';

 import {SelectorScopeRegistry} from './selector_scope';
-import {getConstructorDependencies, isAngularCore, unwrapExpression} from './util';
+import {forwardRefResolver, getConstructorDependencies, isAngularCore, unwrapExpression, unwrapForwardRef} from './util';


forwardRefResolver looks unused, could it be removed

vicb · 2018-07-25T17:21:55Z

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

@@ -103,3 +103,33 @@ export function unwrapExpression(node: ts.Expression): ts.Expression {
  }
  return node;
 }
+
+export function unwrapForwardRef(node: ts.Expression, reflector: ReflectionHost): ts.Expression {


Please add a one liner comment

vicb · 2018-07-25T17:28:25Z

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

@@ -78,6 +78,11 @@ export interface ResolvedValueArray extends Array<ResolvedValue> {}
 */
 type Scope = Map<ts.ParameterDeclaration, ResolvedValue>;

+export enum ImportMode {
+  USE_EXISTING_IMPORT,


nit: enums use PascalCase

use const enum ?

Done. No need for const enum, this never ends up in the browser.

vicb

I added a few minor comments. LGTM otherwise

mary-poppins · 2018-07-25T19:32:24Z

You can preview 6d97ae0 at https://pr25080-6d97ae0.ngbuilds.io/.

mary-poppins · 2018-07-25T23:54:01Z

You can preview ea60fe3 at https://pr25080-ea60fe3.ngbuilds.io/.

mary-poppins · 2018-07-26T21:04:37Z

You can preview 054df5e at https://pr25080-054df5e.ngbuilds.io/.

@ContentChild

@ContentChild[ren] and @ViewChild[ren] can contain a forwardRef() to a type. This commit allows ngtsc to unwrap the forward reference and deal with the node inside. It includes two modes of support for forward reference resolution - a foreign function resolver which understands deeply nested forward references in expressions that are being statically evaluated, and an unwrapForwardRef() function which deals only with top-level nodes. Both will be useful in the future, but for now only unwrapForwardRef() is used.

When ngtsc encounters a reference to a type (for example, a Component type listed in an NgModule declarations array), it traces the import of that type and attempts to determine the best way to refer to it. In the event the type is defined in the same file where a reference is being generated, the identifier of the type is used. If the type was imported, ngtsc has a choice. It can use the identifier from the original import, or it can write a new import to the module where the type came from. ngtsc has a bug currently when it elects to rely on the user's import. When writing a .d.ts file, the user's import may have been elided as the type was not referred to from the type side of the program. Thus, in .d.ts files ngtsc must always assume the import may not exist, and generate a new one. In .js output the import is guaranteed to still exist, so it's preferable for ngtsc to continue using the existing import if one is available. This commit changes how @angular/compiler writes type definitions, and allows it to use a different expression to write a type definition than is used to write the value. This allows ngtsc to specify that types in type definitions should always be imported. A corresponding change to the staticallyResolve() Reference system allows the choice of which type of import to use when generating an Expression from a Reference.

There is a bug in the existing handling for cross-file references. Suppose there are two files, module.ts and component.ts. component.ts declares two components, one of which uses the other. In the Ivy model, this means the component will get a directives: reference to the other in its defineComponent call. That reference is generated by looking at the declared components of the module (in module.ts). However, the way ngtsc tracks this reference, it ends up comparing the identifier of the component in module.ts with the component.ts file, detecting they're not in the same file, and generating a relative import. This commit changes ngtsc to track all identifiers of a reference, including the one by which it is declared. This allows toExpression() to correctly decide that a local reference is okay in component.ts.

@NgModule

ngtsc used to assume that all .d.ts dependencies (that is, third party packages) were imported via an absolute module path. It turns out this assumption isn't valid; some build tools allow relative imports of other compilation units. In the absolute case, ngtsc assumes (and still does) that all referenced types are available through the entrypoint from which an @NgModule was imported. This commit adds support for relative imports, in which case ngtsc will use relative path resolution to determine the imports.

ngtsc used to have a custom ts.CompilerHost which delegated to the plain ts.CompilerHost. There's no need for this wrapper class and it causes issues with CLI integration, so delete it.

compile_strategy() is used to decide whether to build Angular code using ngc (legacy) or ngtsc (local). In order for g3 BUILD rules to switch properly and allow testing of Ivy in g3, they need to import this function. This commit removes the _ prefix which allows the function to be imported.

loadNgStructureAsync() for ngtsc has a bug where it returns a Promise<Promise[]> instead of awaiting the entire array of Promises. This commit uses Promise.all() to await the whole set.

This commit replaces the "not implemented" error when calling listLazyRoutes() with an empty result, which will allow testing in the CLI before listLazyRoutes() is implemented.

mary-poppins · 2018-07-26T21:26:14Z

You can preview 2c0fa0f at https://pr25080-2c0fa0f.ngbuilds.io/.

alxhub · 2018-07-26T22:02:49Z

http://test/OCL:206011842:BASE:206227538:1532642558270:eca7b697

mhevery

Looks reasonable to me. Please take care of comments by others before merging.

all comments have been addressed as far as I can tell.

When ngtsc encounters a reference to a type (for example, a Component type listed in an NgModule declarations array), it traces the import of that type and attempts to determine the best way to refer to it. In the event the type is defined in the same file where a reference is being generated, the identifier of the type is used. If the type was imported, ngtsc has a choice. It can use the identifier from the original import, or it can write a new import to the module where the type came from. ngtsc has a bug currently when it elects to rely on the user's import. When writing a .d.ts file, the user's import may have been elided as the type was not referred to from the type side of the program. Thus, in .d.ts files ngtsc must always assume the import may not exist, and generate a new one. In .js output the import is guaranteed to still exist, so it's preferable for ngtsc to continue using the existing import if one is available. This commit changes how @angular/compiler writes type definitions, and allows it to use a different expression to write a type definition than is used to write the value. This allows ngtsc to specify that types in type definitions should always be imported. A corresponding change to the staticallyResolve() Reference system allows the choice of which type of import to use when generating an Expression from a Reference. PR Close #25080

There is a bug in the existing handling for cross-file references. Suppose there are two files, module.ts and component.ts. component.ts declares two components, one of which uses the other. In the Ivy model, this means the component will get a directives: reference to the other in its defineComponent call. That reference is generated by looking at the declared components of the module (in module.ts). However, the way ngtsc tracks this reference, it ends up comparing the identifier of the component in module.ts with the component.ts file, detecting they're not in the same file, and generating a relative import. This commit changes ngtsc to track all identifiers of a reference, including the one by which it is declared. This allows toExpression() to correctly decide that a local reference is okay in component.ts. PR Close #25080

@NgModule

ngtsc used to assume that all .d.ts dependencies (that is, third party packages) were imported via an absolute module path. It turns out this assumption isn't valid; some build tools allow relative imports of other compilation units. In the absolute case, ngtsc assumes (and still does) that all referenced types are available through the entrypoint from which an @NgModule was imported. This commit adds support for relative imports, in which case ngtsc will use relative path resolution to determine the imports. PR Close #25080

ngtsc used to have a custom ts.CompilerHost which delegated to the plain ts.CompilerHost. There's no need for this wrapper class and it causes issues with CLI integration, so delete it. PR Close #25080

compile_strategy() is used to decide whether to build Angular code using ngc (legacy) or ngtsc (local). In order for g3 BUILD rules to switch properly and allow testing of Ivy in g3, they need to import this function. This commit removes the _ prefix which allows the function to be imported. PR Close #25080

) loadNgStructureAsync() for ngtsc has a bug where it returns a Promise<Promise[]> instead of awaiting the entire array of Promises. This commit uses Promise.all() to await the whole set. PR Close #25080

This commit replaces the "not implemented" error when calling listLazyRoutes() with an empty result, which will allow testing in the CLI before listLazyRoutes() is implemented. PR Close #25080

…r#25080) compile_strategy() is used to decide whether to build Angular code using ngc (legacy) or ngtsc (local). In order for g3 BUILD rules to switch properly and allow testing of Ivy in g3, they need to import this function. This commit removes the _ prefix which allows the function to be imported. PR Close angular#25080

@ContentChild

@ContentChild[ren] and @ViewChild[ren] can contain a forwardRef() to a type. This commit allows ngtsc to unwrap the forward reference and deal with the node inside. It includes two modes of support for forward reference resolution - a foreign function resolver which understands deeply nested forward references in expressions that are being statically evaluated, and an unwrapForwardRef() function which deals only with top-level nodes. Both will be useful in the future, but for now only unwrapForwardRef() is used. PR Close angular#25080

When ngtsc encounters a reference to a type (for example, a Component type listed in an NgModule declarations array), it traces the import of that type and attempts to determine the best way to refer to it. In the event the type is defined in the same file where a reference is being generated, the identifier of the type is used. If the type was imported, ngtsc has a choice. It can use the identifier from the original import, or it can write a new import to the module where the type came from. ngtsc has a bug currently when it elects to rely on the user's import. When writing a .d.ts file, the user's import may have been elided as the type was not referred to from the type side of the program. Thus, in .d.ts files ngtsc must always assume the import may not exist, and generate a new one. In .js output the import is guaranteed to still exist, so it's preferable for ngtsc to continue using the existing import if one is available. This commit changes how @angular/compiler writes type definitions, and allows it to use a different expression to write a type definition than is used to write the value. This allows ngtsc to specify that types in type definitions should always be imported. A corresponding change to the staticallyResolve() Reference system allows the choice of which type of import to use when generating an Expression from a Reference. PR Close angular#25080

There is a bug in the existing handling for cross-file references. Suppose there are two files, module.ts and component.ts. component.ts declares two components, one of which uses the other. In the Ivy model, this means the component will get a directives: reference to the other in its defineComponent call. That reference is generated by looking at the declared components of the module (in module.ts). However, the way ngtsc tracks this reference, it ends up comparing the identifier of the component in module.ts with the component.ts file, detecting they're not in the same file, and generating a relative import. This commit changes ngtsc to track all identifiers of a reference, including the one by which it is declared. This allows toExpression() to correctly decide that a local reference is okay in component.ts. PR Close angular#25080

@NgModule

ngtsc used to assume that all .d.ts dependencies (that is, third party packages) were imported via an absolute module path. It turns out this assumption isn't valid; some build tools allow relative imports of other compilation units. In the absolute case, ngtsc assumes (and still does) that all referenced types are available through the entrypoint from which an @NgModule was imported. This commit adds support for relative imports, in which case ngtsc will use relative path resolution to determine the imports. PR Close angular#25080

ngtsc used to have a custom ts.CompilerHost which delegated to the plain ts.CompilerHost. There's no need for this wrapper class and it causes issues with CLI integration, so delete it. PR Close angular#25080

…r#25080) compile_strategy() is used to decide whether to build Angular code using ngc (legacy) or ngtsc (local). In order for g3 BUILD rules to switch properly and allow testing of Ivy in g3, they need to import this function. This commit removes the _ prefix which allows the function to be imported. PR Close angular#25080

…ular#25080) loadNgStructureAsync() for ngtsc has a bug where it returns a Promise<Promise[]> instead of awaiting the entire array of Promises. This commit uses Promise.all() to await the whole set. PR Close angular#25080

This commit replaces the "not implemented" error when calling listLazyRoutes() with an empty result, which will allow testing in the CLI before listLazyRoutes() is implemented. PR Close angular#25080

angular-automatic-lock-bot · 2019-09-13T21:21:03Z

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 25, 2018

alxhub requested a review from vicb July 25, 2018 00:16

gkalpak reviewed Jul 25, 2018

View reviewed changes

vicb reviewed Jul 25, 2018

View reviewed changes

vicb previously requested changes Jul 25, 2018

View reviewed changes

alxhub force-pushed the ngtsc-july-24 branch from ea60fe3 to 054df5e Compare July 26, 2018 20:55

alxhub changed the title ~~Two ngtsc changes for Material compatibility~~ Batch of ngtsc changes for Material, CLI, g3 compatibility. Jul 26, 2018

alxhub added 8 commits July 26, 2018 14:14

fix(ivy): don't use a custom ts.CompilerHost for ngtsc

0e87937

ngtsc used to have a custom ts.CompilerHost which delegated to the plain ts.CompilerHost. There's no need for this wrapper class and it causes issues with CLI integration, so delete it.

fix(ivy): wait for preanalyze promises in loadNgStructureAsync()

853d20c

loadNgStructureAsync() for ngtsc has a bug where it returns a Promise<Promise[]> instead of awaiting the entire array of Promises. This commit uses Promise.all() to await the whole set.

fix(ivy): don't crash in listLazyRoutes()

2c0fa0f

This commit replaces the "not implemented" error when calling listLazyRoutes() with an empty result, which will allow testing in the CLI before listLazyRoutes() is implemented.

alxhub force-pushed the ngtsc-july-24 branch from 054df5e to 2c0fa0f Compare July 26, 2018 21:17

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

mhevery approved these changes Jul 26, 2018

View reviewed changes

IgorMinar closed this in f902b5e Jul 26, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batch of ngtsc changes for Material, CLI, g3 compatibility. #25080

Batch of ngtsc changes for Material, CLI, g3 compatibility. #25080

alxhub commented Jul 25, 2018

mary-poppins commented Jul 25, 2018

gkalpak Jul 25, 2018

alxhub Jul 26, 2018

gkalpak Jul 25, 2018

alxhub Jul 26, 2018

vicb Jul 25, 2018

alxhub Jul 26, 2018

vicb Jul 25, 2018

alxhub Jul 26, 2018

vicb Jul 25, 2018

vicb Jul 25, 2018

alxhub Jul 26, 2018

vicb left a comment

mary-poppins commented Jul 25, 2018

mary-poppins commented Jul 25, 2018

mary-poppins commented Jul 26, 2018

mary-poppins commented Jul 26, 2018

alxhub commented Jul 26, 2018

mhevery left a comment

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

Batch of ngtsc changes for Material, CLI, g3 compatibility. #25080

Batch of ngtsc changes for Material, CLI, g3 compatibility. #25080

Conversation

alxhub commented Jul 25, 2018

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

vicb left a comment

Choose a reason for hiding this comment

mary-poppins commented Jul 25, 2018

mary-poppins commented Jul 25, 2018

mary-poppins commented Jul 26, 2018

mary-poppins commented Jul 26, 2018

alxhub commented Jul 26, 2018

mhevery left a comment

Choose a reason for hiding this comment

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