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

JoostK · 2019-10-20T21:39:16Z

This PR contains several commits to introduce schematic-like migration in ngcc. More details can be found in each commit description.

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

JoostK · 2019-10-22T19:03:34Z

After having tested within the ngcc-validation project in angular/ngcc-validation#439 (thanks @gkalpak!) two issues were identified:

synthetic decorators did get emitted as metadata, which should not be the case. This has been addressed by preventing the internal decorators cache of Esm2015ReflectionHost from being clobbered with outside changes, such as injecting synthetic decorators. With this change, synthetic decorators no longer leak into the real decorators so won't be emitted as metadata.
Abstract directives could be migrated despite already having a @Directive decorator, causing an error. This error was addressed by updating hasDirectiveDEcorator to also consider MetadataReader.isAbstractDirective, and tests have been adjusted to assert that no errors occurred. Note that this change is obsolete once fix(ivy): support abstract directives in template type checking #33131 has landed.

I am fairly confident that the migrations now operate as they should. The two packages that regressed in the test run are now building successfully for me locally.

gkalpak

Great stuff 💯

_{I wish there was some way to share logic with the schematics migrations, but I guess the "infrastructure" is too different for that 🤷‍♂}

packages/compiler-cli/ngcc/src/analysis/migration_host.ts

gkalpak · 2019-10-23T15:26:17Z

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

+  /**
+   * Determines whether the provided class in within scope of the entry-point that is currently
+   * being compiled.
+   * @param clazz the class for which to be determine whether it is within the current entry-point.


to be determine --> to be determined/to determine

gkalpak · 2019-10-23T15:53:34Z

packages/compiler-cli/src/ngtsc/reflection/src/host.ts

@@ -32,7 +33,8 @@ export interface Decorator {
  import : Import | null;

  /**
-   * TypeScript reference to the decorator itself.
+   * TypeScript reference to the decorator itself, or the node for which the decorator was
+   * synthesized in ngcc.


What is the point of having node point to the node for which the decorator was synthesized? Is it used anywhere?

Decorator.node is primarily used for producing diagnostics (for its location) so it would be quite cumbersome if this could be null. For synthesized decorators we don't expect diagnostics to be created, so from that perspective this shouldn't matter much, however I liked using the migrated class' identifier as a fallback.

packages/compiler-cli/ngcc/test/analysis/decoration_analyzer_spec.ts

packages/compiler-cli/ngcc/src/host/esm2015_host.ts

gkalpak · 2019-10-23T16:39:41Z

packages/compiler-cli/ngcc/test/migrations/undecorated_parent_migration_spec.ts

+         expect(decorator.name).toEqual('Directive');
+         expect(decorator.identifier).toBeNull('The decorator must be synthesized');
+         expect(decorator.import).toEqual({from: '@angular/core', name: 'Directive'});
+         expect(decorator.args !.length).toEqual(0);


Could you add some assertions about what happens to IntermediateClass.

gkalpak · 2019-10-23T16:46:29Z

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

+ * `Directive.providers`/`Component.viewProviders` also have an `@Injectable()` decorator. This
+ * decorator is now mandatory, as otherwise the compiler would not compile an injectable definition
+ * for the service. This is unlike View Engine, where having just an unrelated decorator may have
+ * been sufficient for the service to become injectable.


This paragraph confuses me a bit. By reading it, I thought that we need the @Injectable decorator no matter what. By looking at the code, it seems that @Injectable is needed only if the class does not have @Directive, @Component or @Pipe.

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.

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.

JoostK · 2019-10-24T19:08:31Z

Closing as these commits are part of #33362 to be landed at once.

angular-automatic-lock-bot · 2019-11-24T16:10:48Z

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.}

JoostK added feature Issue that requests a new feature target: major This PR is targeted for the next major release comp: ivy labels Oct 20, 2019

ngbot bot modified the milestone: needsTriage Oct 20, 2019

googlebot added the cla: yes label Oct 20, 2019

trotyl reviewed Oct 21, 2019

View reviewed changes

packages/compiler-cli/ngcc/src/migrations/missing_injectable_migration.ts Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

JoostK force-pushed the ngcc-missing-injectable-migration branch 2 times, most recently from b4659f9 to e335292 Compare October 21, 2019 23:28

gkalpak added a commit to gkalpak/ngcc-validation that referenced this pull request Oct 22, 2019

DO NOT MERGE - Try PR angular/angular#33279

d73fed8

gkalpak mentioned this pull request Oct 22, 2019

DO NOT MERGE - Try PR https://github.com/angular/angular/pull/33279 angular/ngcc-validation#439

Closed

JoostK force-pushed the ngcc-missing-injectable-migration branch from e335292 to 0be9bde Compare October 22, 2019 18:47

JoostK marked this pull request as ready for review October 22, 2019 18:53

JoostK requested review from a team as code owners October 22, 2019 18:53

gkalpak approved these changes Oct 23, 2019

View reviewed changes

JoostK added 8 commits October 23, 2019 21:50

fixup! fix(ngcc): prevent reflected decorators from being clobbered

fd8b1a8

fixup! feat(ngcc): migrate services that are missing @Injectable()

be384d1

fixup! refactor(ngcc): rework undecorated parent migration

72b67d6

JoostK force-pushed the ngcc-missing-injectable-migration branch from 0be9bde to ebf596c Compare October 23, 2019 19:51

JoostK closed this Oct 24, 2019

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

JoostK commented Oct 20, 2019

This comment has been minimized.

JoostK commented Oct 22, 2019 •

edited

gkalpak left a comment

gkalpak Oct 23, 2019 •

edited

gkalpak Oct 23, 2019

JoostK Oct 23, 2019

gkalpak Oct 23, 2019

gkalpak Oct 23, 2019

JoostK commented Oct 24, 2019

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

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

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

Conversation

JoostK commented Oct 20, 2019

This comment has been minimized.

JoostK commented Oct 22, 2019 • edited

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak Oct 23, 2019 • edited

Choose a reason for hiding this comment

gkalpak Oct 23, 2019

Choose a reason for hiding this comment

JoostK Oct 23, 2019

Choose a reason for hiding this comment

gkalpak Oct 23, 2019

Choose a reason for hiding this comment

gkalpak Oct 23, 2019

Choose a reason for hiding this comment

JoostK commented Oct 24, 2019

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

JoostK commented Oct 22, 2019 •

edited

gkalpak Oct 23, 2019 •

edited