All ngcc migrations #33362

alxhub · 2019-10-23T19:01:45Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

googlebot · 2019-10-23T19:01:53Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

JoostK

First round of comments (for today). Overall this is a bit light on tests.

packages/compiler-cli/ngcc/src/migrations/undecorated_child_migration.ts

JoostK · 2019-10-23T20:00:15Z

packages/compiler-cli/ngcc/src/migrations/undecorated_child_migration.ts

+    // This migration looks at NgModules and considers the directives (and pipes) it declares.
+    // It verifies that these classes have decorators.


I thought we discussed cases where undecorated classes might exist in libraries without registering them in an NgModule. Although this sounds like a pathological use case to me, I thought @petebacondarwin was able to convince you 😉

I thought through this and came to the realization that it's not actually possible. VE would consider those classes as decorated, and would generate an error ("could not determine NgModule for ...") if they were not part of a module.

So I don't think that case actually occurs in reality.

JoostK · 2019-10-23T20:19:08Z

packages/compiler-cli/ngcc/test/integration/util.ts

+  }
+}
+
+function compileNodeModuleToFs(fs: FileSystem, pkgName: string, pkg: Package): void {


I love this, this is super helpful for having easy to write and easy to maintain tests 😍

I want to expand it over time, so you can start with TS code and ask for a node_module layout with some or all of the possible formats, rolled up to UMD, maybe even minified, etc.

JoostK · 2019-10-23T20:20:31Z

packages/compiler-cli/ngcc/test/integration/util.ts

+        fs.resolve(`/node_modules/${pkgName}${inFileBase}.d.ts`),
+        compileFs.readFile(compileFs.resolve(`${inFileBase}.d.ts`)));
+    const jsContents = compileFs.readFile(compileFs.resolve(`${inFileBase}.js`));
+    fs.writeFile(fs.resolve(`/node_modules/${pkgName}${inFileBase}.js`), jsContents);


Could you ensure there's a slash separating inFileBase from pkgName?

JoostK · 2019-10-23T20:22:02Z

packages/compiler-cli/ngcc/test/integration/util.ts

+    this.fs.writeFile(this.fs.resolve(fileName), data);
+  }
+
+  getCurrentDirectory(): string { return '/'; }


Consider using this.fs.pwd();.

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

JoostK · 2019-10-23T20:41:12Z

packages/core/src/render3/features/copy_definition_feature.ts

+];
+
+/**
+ * Merges the definition from a super class to a sub class.


Mentioning merge semantics is confusing here, given the feature's name.

packages/core/src/render3/features/copy_definition_feature.ts

JoostK · 2019-10-23T20:45:41Z

packages/compiler/src/render3/view/compiler.ts

@@ -104,6 +104,9 @@ function addFeatures(
  if (meta.usesInheritance) {
    features.push(o.importExpr(R3.InheritDefinitionFeature));
  }
+  if (meta.fullInheritance) {


If CopyDefinitionFeature always requires InheritDefinitionFeature (which I believe is the case judging on the feature's impl), I would prefer that to be explicit:

if (meta.fullInheritance) { features.push(o.importExpr(R3.InheritDefinitionFeature)); features.push(o.importExpr(R3.CopyDefinitionFeature)); } else if (meta.usesInheritance) { features.push(o.importExpr(R3.InheritDefinitionFeature)); }

I think in general we need to make inheritance an enum:

enum R3DecoratorInheritance { None, Limited, /* decorated fields are inherited... inputs, outputs, etc) */ Full, /* all aspects of the base class are inherited */ }

@kara please review the last two commits, especially the runtime side.

googlebot · 2019-10-24T00:24:50Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

gkalpak

I'd also like to see some more tests. Otherwise, everything looks reasonable (afaict 😁) 👍

gkalpak · 2019-10-24T08:39:56Z

packages/compiler-cli/ngcc/src/migrations/utils.ts

@@ -23,6 +23,14 @@ export function hasDirectiveDecorator(host: MigrationHost, clazz: ClassDeclarati
  return host.metadata.getDirectiveMetadata(ref) !== null || host.metadata.isAbstractDirective(ref);
 }

+/**
+ * Returns true if the `clazz` is decorated as a `Directive` or `Component`.


as a `Directive` or `Component` --> as a `Pipe`

gkalpak · 2019-10-24T08:51:58Z

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

+  NONE = 0x0,
+
+  /**
+   * Indicates that this decorator should fully inherited from its parent at runtime. In addition


inherited --> inherit(?)

gkalpak · 2019-10-24T08:55:42Z

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

+   * Its primary effect is to cause the `CopyDefinitionFeature` to be applied to the definition
+   * being compiled. See that class for more information.
+   */
+  FULL_INHERITANCE = 0x00000001,


Why not 0x1 (or 0x00000000 above 😁)?

Gotta make room for the other flags!

Honestly, it's so things line up nicely when we reach 0x10. Though that'll never happen.

gkalpak · 2019-10-24T09:06:55Z

packages/compiler-cli/ngcc/src/migrations/undecorated_child_migration.ts

+
+  maybeMigrate(ref: Reference<ClassDeclaration>, host: MigrationHost): ts.Diagnostic|null {
+    if (hasDirectiveDecorator(host, ref.node) || hasPipeDecorator(host, ref.node)) {
+      // Stop if one of the classes in the chain is actually decorated with @Directive.


May I add or @Component or @Pipe? 😛

gkalpak · 2019-10-24T09:10:03Z

packages/compiler-cli/ngcc/src/migrations/undecorated_child_migration.ts

+    }
+
+
+    // Success!


Yay 🎉 👏 🍾

gkalpak · 2019-10-24T09:10:49Z

packages/compiler-cli/ngcc/src/migrations/undecorated_child_migration.ts

+
+    // Examine each of the declarations to see if it needs to be migrated.
+    for (const decl of moduleMeta.declarations) {
+      const diag = this.maybeMigrate(decl, host);


It seems that this can never return anything but null.

Failure is not an option.

gkalpak · 2019-10-24T09:25:07Z

packages/compiler-cli/ngcc/test/integration/util.ts

+  [relPath: string]: string;
+};
+
+export function genNodeModules(def: NodeModulesDef): void {


Please add a comment here and/or on compileNodeModuleToFs() to explain what the function does.

kara

A few small things

packages/core/src/render3/features/copy_definition_feature.ts

packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts

packages/core/src/render3/features/copy_definition_feature.ts

googlebot · 2019-10-24T19:21:40Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

kara

LGTM

ngcc has an internal cache of computed decorator information for reflected classes, which could previously be mutated by consumers of the reflection host. With the ability to inject synthesized decorators, such decorators would inadvertently be added into the array of decorators that was owned by the internal cache of the reflection host, incorrectly resulting in synthesized decorators to be considered real decorators on a class. This commit fixes the issue by cloning the cached array before returning it.

A class that is provided as Angular service is required to have an `@Injectable()` decorator so that the compiler generates its injectable definition for the runtime. Applications are automatically migrated using the "missing-injectable" schematic, however libraries built for older version of Angular may not yet satisfy this requirement. This commit ports the "missing-injectable" schematic to a migration that is ran when ngcc is processing a library. This ensures that any service that is provided from an NgModule or Directive/Component will have an `@Injectable()` decorator.

kara · 2019-10-25T00:24:54Z

merge-assistance: global approval

In ngcc's migration system, synthetic decorators can be injected into a compilation to ensure that certain classes are compiled with Angular logic, where the original library code did not include the necessary decorators. Prior to this change, synthesized decorators would have a fake AST structure as associated node and a made-up identifier. In theory, this may introduce issues downstream: 1) a decorator's node is used for diagnostics, so it must have position information. Having fake AST nodes without a position is therefore a problem. Note that this is currently not a problem in practice, as injected synthesized decorators would not produce any diagnostics. 2) the decorator's identifier should refer to an imported symbol. Therefore, it is required that the symbol is actually imported. Moreover, bundle formats such as UMD and CommonJS use namespaces for imports, so a bare `ts.Identifier` would not be suitable to use as identifier. This was also not a problem in practice, as the identifier is only used in the `setClassMetadata` generated code, which is omitted for synthetically injected decorators. To remedy these potential issues, this commit makes a decorator's identifier optional and switches its node over from a fake AST structure to the class' name.

Previously, the (currently disabled) undecorated parent migration in ngcc would produce errors when a base class could not be determined statically or when a class extends from a class in another package. This is not ideal, as it would cause the library to fail compilation without a workaround, whereas those problems are not guaranteed to cause issues. Additionally, inheritance chains were not handled. This commit reworks the migration to address these limitations.

When upgrading an Angular application to a new version using the Angular CLI, built-in schematics are being run to update user code from deprecated patterns to the new way of working. For libraries that have been built for older versions of Angular however, such schematics have not been executed which means that deprecated code patterns may still be present, potentially resulting in incorrect behavior. Some of the logic of schematics has been ported over to ngcc migrations, which are automatically run on libraries. These migrations achieve the same goal of the regular schematics, but operating on published library sources instead of used code.

@directive

This commit adds CopyDefinitionFeature, which supports the case where an entire decorator (@component or @directive) is inherited from parent to child. The existing inheritance feature, InheritDefinitionFeature, supports merging of parent and child definitions when both were originally present. This merges things like inputs, outputs, host bindings, etc. CopyDefinitionFeature, on the other hand, compensates for a definition that was missing entirely on the child class, by copying fields that aren't ordinarily inherited (like the template function itself). This feature is intended to only be used as part of ngcc code generation.

@directive

In Angular View Engine, there are two kinds of decorator inheritance: 1) both the parent and child classes have decorators This case is supported by InheritDefinitionFeature, which merges some fields of the definitions (such as the inputs or queries). 2) only the parent class has a decorator If the child class is missing a decorator, the compiler effectively behaves as if the parent class' decorator is applied to the child class as well. This is the "undecorated child" scenario, and this commit adds a migration to ngcc to support this pattern in Ivy. This migration has 2 phases. First, the NgModules of the application are scanned for classes in 'declarations' which are missing decorators, but whose base classes do have decorators. These classes are the undecorated children. This scan is performed recursively, so even if a declared class has a base class that itself inherits a decorator, this case is handled. Next, a synthetic decorator (either @component or @directive) is created on the child class. This decorator copies some critical information such as 'selector' and 'exportAs', as well as supports any decorated fields (@input, etc). A flag is passed to the decorator compiler which causes a special feature `CopyDefinitionFeature` to be included on the compiled definition. This feature copies at runtime the remaining aspects of the parent definition which `InheritDefinitionFeature` does not handle, completing the "full" inheritance of the child class' decorator from its parent class.

googlebot · 2019-10-25T00:29:54Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

AndrewKushnir · 2019-10-25T00:34:01Z

AndrewKushnir · 2019-10-25T00:34:58Z

FYI, there are build failures in g3 with the changes from this PR, adding "blocked" label.

googlebot · 2019-10-25T00:52:24Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

AndrewKushnir · 2019-10-25T00:54:18Z

New set of presubmits:

AndrewKushnir · 2019-10-25T16:15:33Z

FYI, VE and Ivy presubmits are successful.

A class that is provided as Angular service is required to have an `@Injectable()` decorator so that the compiler generates its injectable definition for the runtime. Applications are automatically migrated using the "missing-injectable" schematic, however libraries built for older version of Angular may not yet satisfy this requirement. This commit ports the "missing-injectable" schematic to a migration that is ran when ngcc is processing a library. This ensures that any service that is provided from an NgModule or Directive/Component will have an `@Injectable()` decorator. PR Close #33362

In ngcc's migration system, synthetic decorators can be injected into a compilation to ensure that certain classes are compiled with Angular logic, where the original library code did not include the necessary decorators. Prior to this change, synthesized decorators would have a fake AST structure as associated node and a made-up identifier. In theory, this may introduce issues downstream: 1) a decorator's node is used for diagnostics, so it must have position information. Having fake AST nodes without a position is therefore a problem. Note that this is currently not a problem in practice, as injected synthesized decorators would not produce any diagnostics. 2) the decorator's identifier should refer to an imported symbol. Therefore, it is required that the symbol is actually imported. Moreover, bundle formats such as UMD and CommonJS use namespaces for imports, so a bare `ts.Identifier` would not be suitable to use as identifier. This was also not a problem in practice, as the identifier is only used in the `setClassMetadata` generated code, which is omitted for synthetically injected decorators. To remedy these potential issues, this commit makes a decorator's identifier optional and switches its node over from a fake AST structure to the class' name. PR Close #33362

Previously, the (currently disabled) undecorated parent migration in ngcc would produce errors when a base class could not be determined statically or when a class extends from a class in another package. This is not ideal, as it would cause the library to fail compilation without a workaround, whereas those problems are not guaranteed to cause issues. Additionally, inheritance chains were not handled. This commit reworks the migration to address these limitations. PR Close #33362

When upgrading an Angular application to a new version using the Angular CLI, built-in schematics are being run to update user code from deprecated patterns to the new way of working. For libraries that have been built for older versions of Angular however, such schematics have not been executed which means that deprecated code patterns may still be present, potentially resulting in incorrect behavior. Some of the logic of schematics has been ported over to ngcc migrations, which are automatically run on libraries. These migrations achieve the same goal of the regular schematics, but operating on published library sources instead of used code. PR Close #33362

@directive

This commit adds CopyDefinitionFeature, which supports the case where an entire decorator (@component or @directive) is inherited from parent to child. The existing inheritance feature, InheritDefinitionFeature, supports merging of parent and child definitions when both were originally present. This merges things like inputs, outputs, host bindings, etc. CopyDefinitionFeature, on the other hand, compensates for a definition that was missing entirely on the child class, by copying fields that aren't ordinarily inherited (like the template function itself). This feature is intended to only be used as part of ngcc code generation. PR Close #33362

@directive

In Angular View Engine, there are two kinds of decorator inheritance: 1) both the parent and child classes have decorators This case is supported by InheritDefinitionFeature, which merges some fields of the definitions (such as the inputs or queries). 2) only the parent class has a decorator If the child class is missing a decorator, the compiler effectively behaves as if the parent class' decorator is applied to the child class as well. This is the "undecorated child" scenario, and this commit adds a migration to ngcc to support this pattern in Ivy. This migration has 2 phases. First, the NgModules of the application are scanned for classes in 'declarations' which are missing decorators, but whose base classes do have decorators. These classes are the undecorated children. This scan is performed recursively, so even if a declared class has a base class that itself inherits a decorator, this case is handled. Next, a synthetic decorator (either @component or @directive) is created on the child class. This decorator copies some critical information such as 'selector' and 'exportAs', as well as supports any decorated fields (@input, etc). A flag is passed to the decorator compiler which causes a special feature `CopyDefinitionFeature` to be included on the compiled definition. This feature copies at runtime the remaining aspects of the parent definition which `InheritDefinitionFeature` does not handle, completing the "full" inheritance of the child class' decorator from its parent class. PR Close #33362

angular-automatic-lock-bot · 2019-11-25T16:11:40Z

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: no label Oct 23, 2019

JoostK reviewed Oct 23, 2019

View reviewed changes

alxhub force-pushed the ngcc/migrations branch from 0750fc8 to 4f22811 Compare October 23, 2019 23:58

alxhub added cla: yes and removed cla: no labels Oct 24, 2019

alxhub added target: major This PR is targeted for the next major release action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 24, 2019

alxhub requested a review from kara October 24, 2019 00:25

gkalpak reviewed Oct 24, 2019

View reviewed changes

kara requested changes Oct 24, 2019

View reviewed changes

packages/core/src/render3/features/copy_definition_feature.ts Show resolved Hide resolved

packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts Show resolved Hide resolved

packages/core/src/render3/features/copy_definition_feature.ts Outdated Show resolved Hide resolved

kara marked this pull request as ready for review October 24, 2019 18:59

kara requested review from a team as code owners October 24, 2019 18:59

JoostK mentioned this pull request Oct 24, 2019

feat(ngcc): execute schematic-like migrations in ngcc #33279

Closed

alxhub force-pushed the ngcc/migrations branch from 4f22811 to 5144be2 Compare October 24, 2019 19:21

googlebot added cla: no and removed cla: yes labels Oct 24, 2019

kara approved these changes Oct 24, 2019

View reviewed changes

alxhub force-pushed the ngcc/migrations branch 2 times, most recently from ae402b5 to e015bdd Compare October 24, 2019 22:42

JoostK added 2 commits October 24, 2019 16:19

alxhub force-pushed the ngcc/migrations branch from e015bdd to d3998f0 Compare October 24, 2019 23:29

alxhub added cla: yes and removed cla: no labels Oct 24, 2019

kara added the action: presubmit The PR is in need of a google3 presubmit label Oct 25, 2019

JoostK and others added 5 commits October 24, 2019 17:29

alxhub force-pushed the ngcc/migrations branch from d3998f0 to f1b6302 Compare October 25, 2019 00:29

googlebot added cla: no and removed cla: yes labels Oct 25, 2019

AndrewKushnir added the state: blocked label Oct 25, 2019

AndrewKushnir added cla: yes and removed cla: no labels Oct 25, 2019

AndrewKushnir removed state: blocked action: presubmit The PR is in need of a google3 presubmit labels Oct 25, 2019

AndrewKushnir closed this in 0de2dbf Oct 25, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 25, 2019

		// This migration looks at NgModules and considers the directives (and pipes) it declares.
		// It verifies that these classes have decorators.

All ngcc migrations #33362

All ngcc migrations #33362

Conversation

alxhub commented Oct 23, 2019

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

googlebot commented Oct 23, 2019

JoostK 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

googlebot commented Oct 24, 2019

gkalpak 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

alxhub Oct 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kara left a comment

Choose a reason for hiding this comment

googlebot commented Oct 24, 2019

kara left a comment

Choose a reason for hiding this comment

kara commented Oct 25, 2019

googlebot commented Oct 25, 2019

AndrewKushnir commented Oct 25, 2019

AndrewKushnir commented Oct 25, 2019

googlebot commented Oct 25, 2019

AndrewKushnir commented Oct 25, 2019

AndrewKushnir commented Oct 25, 2019

angular-automatic-lock-bot bot commented Nov 25, 2019

alxhub Oct 24, 2019 •

edited