From 0577bf0e3e2bf5dfa3c4189c414914647187bc8d Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Mon, 4 May 2020 21:58:42 +0200 Subject: [PATCH] refactor: enable ng update migrations for v10 (#36921) Enables the `ng update` migrations for v10. Status for individual migrations: **undecorated-classes-with-di**. This migration dealt exlusively with inherited constructors and cases where a derived component was undecorated. In those cases, the migration added `@Directive()` or copied the inherited decorator to the derived class. We don't need to run this migration again because ngtsc throws if constructor is inherited from an undecorated class. Also ngtsc will throw if a NgModule references an undecorated class in the declarations. ***undecorated-classes-with-decorated-fields*** This migration exclusively deals with undecorated classes that use Angular features but are not decorated. Angular features include the use of lifecycle hooks or class fields with Angular decorators, such as `@Input()`. We want to re-run this migration in v10 as we will disable the compatibility code in ngtsc that detects such undecorated classes as `@Directive`. **module-with-providers**: This migration adds an explicit generic type to `ModuleWithProviders`. As of v10, the generic type is required, so we need to re-run the migration again. **renderer-to-renderer2**: We don't need to re-run that migration again as the renderer has been already removed in v9. **missing-injectable**: This migration is exclusively concerned with undecorated providers referenced in an `NgModule`. We should re-run that migration again as we don't have proper backsliding prevention for this yet. We can consider adding an error in ngtsc for v10, or v11. In either way, we should re-run the migration. **dynamic-queries**: We ran this one in v9 to reduce code complexity in projects. Instead of explicitly passing `static: false`, not passing any object literal has the same semantics. We don't need to re-run the migration again since there is no good way to prevent backsliding and we cannot always run this migration for future versions (as some apps might actually intentionally use the explicit `static: false` option). PR Close #36921 --- .../migration-tests/providers-test-module.ts | 5 +- .../providers-test-module_expected.ts | 4 +- .../undecorated-base-classes-with-di.ts | 42 ------------- ...decorated-base-classes-with-di_expected.ts | 45 -------------- .../undecorated-classes-with-fields.ts | 16 +++++ ...ndecorated-classes-with-fields_expected.ts | 19 +++++- .../undecorated-derived-classes.ts | 31 ---------- .../undecorated-derived-classes_expected.ts | 62 ------------------- integration/ng_update_migrations/test.js | 4 +- packages/core/schematics/migrations.json | 26 ++++---- .../test/missing_injectable_migration_spec.ts | 2 +- .../module_with_providers_migration_spec.ts | 2 +- ...es_with_decorated_fields_migration_spec.ts | 2 +- 13 files changed, 54 insertions(+), 206 deletions(-) delete mode 100644 integration/ng_update_migrations/src/app/migration-tests/undecorated-base-classes-with-di.ts delete mode 100644 integration/ng_update_migrations/src/app/migration-tests/undecorated-base-classes-with-di_expected.ts delete mode 100644 integration/ng_update_migrations/src/app/migration-tests/undecorated-derived-classes.ts delete mode 100644 integration/ng_update_migrations/src/app/migration-tests/undecorated-derived-classes_expected.ts diff --git a/integration/ng_update_migrations/src/app/migration-tests/providers-test-module.ts b/integration/ng_update_migrations/src/app/migration-tests/providers-test-module.ts index c56cb801d9..177c7879f2 100644 --- a/integration/ng_update_migrations/src/app/migration-tests/providers-test-module.ts +++ b/integration/ng_update_migrations/src/app/migration-tests/providers-test-module.ts @@ -1,4 +1,4 @@ -import {Component, Directive, NgModule, NgZone} from '@angular/core'; +import {Component, Directive, NgModule, NgZone, Injectable} from '@angular/core'; export class ComponentTypeProvider {} export class ComponentDontNeedCase {} @@ -28,8 +28,7 @@ export class ProvidersTestDirective {} export class TypeCase {} -// undecorated-classes-with-di migration will add "@Injectable" here -// because the constructor is inherited in "ProvideCase". +@Injectable() class BaseProviderCase { constructor(zone: NgZone) {} } diff --git a/integration/ng_update_migrations/src/app/migration-tests/providers-test-module_expected.ts b/integration/ng_update_migrations/src/app/migration-tests/providers-test-module_expected.ts index 0d6d4ac2c5..18aebf4c57 100644 --- a/integration/ng_update_migrations/src/app/migration-tests/providers-test-module_expected.ts +++ b/integration/ng_update_migrations/src/app/migration-tests/providers-test-module_expected.ts @@ -1,4 +1,4 @@ -import { Component, Directive, NgModule, NgZone, Injectable } from '@angular/core'; +import {Component, Directive, NgModule, NgZone, Injectable} from '@angular/core'; @Injectable() export class ComponentTypeProvider {} @@ -34,8 +34,6 @@ export class ProvidersTestDirective {} @Injectable() export class TypeCase {} -// undecorated-classes-with-di migration will add "@Injectable" here -// because the constructor is inherited in "ProvideCase". @Injectable() class BaseProviderCase { constructor(zone: NgZone) {} diff --git a/integration/ng_update_migrations/src/app/migration-tests/undecorated-base-classes-with-di.ts b/integration/ng_update_migrations/src/app/migration-tests/undecorated-base-classes-with-di.ts deleted file mode 100644 index 446fcc4738..0000000000 --- a/integration/ng_update_migrations/src/app/migration-tests/undecorated-base-classes-with-di.ts +++ /dev/null @@ -1,42 +0,0 @@ -import {Directive, NgModule, NgZone} from '@angular/core'; - -export class BaseClass1 { - constructor(zone: NgZone) {} -} - -@Directive({selector: 'sel'}) -export class DirectiveWithInheritedCtor extends BaseClass1 {} - -export class BaseClass2 { - constructor(zone: NgZone) {} -} - -@Directive({selector: 'sel'}) -export class DirectiveWithExplicitCtor extends BaseClass2 { - constructor(zone: NgZone) { - super(zone); - } -} - -export class BaseClassWithoutCtor {} - -@Directive({selector: 'sel'}) -export class DirectiveWithoutInheritedCtor extends BaseClassWithoutCtor {} - -export class BaseClass3 { - constructor(zone: NgZone) {} -} -export class PassThroughClass extends BaseClass3 {} - -@Directive({selector: 'sel'}) -export class DirectiveWithInheritedCtorAndClassesInBetween extends PassThroughClass {} - -@NgModule({ - declarations: [ - DirectiveWithInheritedCtor, - DirectiveWithoutInheritedCtor, - DirectiveWithExplicitCtor, - DirectiveWithInheritedCtorAndClassesInBetween, - ] -}) -export class TestModule {} diff --git a/integration/ng_update_migrations/src/app/migration-tests/undecorated-base-classes-with-di_expected.ts b/integration/ng_update_migrations/src/app/migration-tests/undecorated-base-classes-with-di_expected.ts deleted file mode 100644 index 971b8a98b8..0000000000 --- a/integration/ng_update_migrations/src/app/migration-tests/undecorated-base-classes-with-di_expected.ts +++ /dev/null @@ -1,45 +0,0 @@ -import {Directive, NgModule, NgZone} from '@angular/core'; - -@Directive() -export class BaseClass1 { - constructor(zone: NgZone) {} -} - -@Directive({selector: 'sel'}) -export class DirectiveWithInheritedCtor extends BaseClass1 {} - -export class BaseClass2 { - constructor(zone: NgZone) {} -} - -@Directive({selector: 'sel'}) -export class DirectiveWithExplicitCtor extends BaseClass2 { - constructor(zone: NgZone) { - super(zone); - } -} - -export class BaseClassWithoutCtor {} - -@Directive({selector: 'sel'}) -export class DirectiveWithoutInheritedCtor extends BaseClassWithoutCtor {} - -@Directive() -export class BaseClass3 { - constructor(zone: NgZone) {} -} -@Directive() -export class PassThroughClass extends BaseClass3 {} - -@Directive({selector: 'sel'}) -export class DirectiveWithInheritedCtorAndClassesInBetween extends PassThroughClass {} - -@NgModule({ - declarations: [ - DirectiveWithInheritedCtor, - DirectiveWithoutInheritedCtor, - DirectiveWithExplicitCtor, - DirectiveWithInheritedCtorAndClassesInBetween, - ] -}) -export class TestModule {} diff --git a/integration/ng_update_migrations/src/app/migration-tests/undecorated-classes-with-fields.ts b/integration/ng_update_migrations/src/app/migration-tests/undecorated-classes-with-fields.ts index c3ea7d460e..dd81dd2187 100644 --- a/integration/ng_update_migrations/src/app/migration-tests/undecorated-classes-with-fields.ts +++ b/integration/ng_update_migrations/src/app/migration-tests/undecorated-classes-with-fields.ts @@ -49,6 +49,10 @@ export class MyCompBase extends MyCompSuperBase {} }) export class MyComp extends MyCompBase {} +@Component({ + selector: 'my-comp', + template: '', +}) export class WrappedMyComp extends MyComp {} @NgModule({declarations: [MyComp, WrappedMyComp]}) @@ -60,3 +64,15 @@ export class AbstractDir {} export class DerivedAbstractDir extends AbstractDir {} export class WrappedDerivedAbstractDir extends DerivedAbstractDir {} + +export class UndecoratedService { + ngOnDestroy() {} +} + +export class UndecoratedPipeBase { + ngOnDestroy() {} +} + +export class WithDirectiveLifecycleHook { + ngOnInit() {} +} diff --git a/integration/ng_update_migrations/src/app/migration-tests/undecorated-classes-with-fields_expected.ts b/integration/ng_update_migrations/src/app/migration-tests/undecorated-classes-with-fields_expected.ts index ebc267f02a..664188b826 100644 --- a/integration/ng_update_migrations/src/app/migration-tests/undecorated-classes-with-fields_expected.ts +++ b/integration/ng_update_migrations/src/app/migration-tests/undecorated-classes-with-fields_expected.ts @@ -56,8 +56,8 @@ export class MyCompBase extends MyCompSuperBase {} export class MyComp extends MyCompBase {} @Component({ - selector: 'my-comp', - template: '', + selector: 'my-comp', + template: '', }) export class WrappedMyComp extends MyComp {} @@ -72,3 +72,18 @@ export class DerivedAbstractDir extends AbstractDir {} @Directive() export class WrappedDerivedAbstractDir extends DerivedAbstractDir {} + +// TODO: Add Angular decorator. +export class UndecoratedService { + ngOnDestroy() {} +} + +// TODO: Add Angular decorator. +export class UndecoratedPipeBase { + ngOnDestroy() {} +} + +@Directive() +export class WithDirectiveLifecycleHook { + ngOnInit() {} +} diff --git a/integration/ng_update_migrations/src/app/migration-tests/undecorated-derived-classes.ts b/integration/ng_update_migrations/src/app/migration-tests/undecorated-derived-classes.ts deleted file mode 100644 index f68ab1c697..0000000000 --- a/integration/ng_update_migrations/src/app/migration-tests/undecorated-derived-classes.ts +++ /dev/null @@ -1,31 +0,0 @@ -import {Directive, NgModule, NgZone} from '@angular/core'; -import {CheckboxControlValueAccessor} from '@angular/forms'; -import {BaseComponentFromOtherFile} from './base-component'; - - -@Directive({ - selector: 'my-base-dir', - providers: [{provide: NgZone, useValue: null}] -}) -export class BaseDirective {} - -export class DerivedDirective extends BaseDirective {} - -export class DerivedDirectiveFromNodeModules extends CheckboxControlValueAccessor {} - -export class DerivedComponentFromOtherSourceFile extends BaseComponentFromOtherFile {} - -@NgModule({ - declarations: [ - DerivedDirective, - DerivedDirectiveFromNodeModules, - DerivedComponentFromOtherSourceFile, - ], -}) -export class TestModule {} - - -@NgModule({ - declarations: [BaseDirective], -}) -export class BaseClassesModule {} diff --git a/integration/ng_update_migrations/src/app/migration-tests/undecorated-derived-classes_expected.ts b/integration/ng_update_migrations/src/app/migration-tests/undecorated-derived-classes_expected.ts deleted file mode 100644 index 3b2042b8ea..0000000000 --- a/integration/ng_update_migrations/src/app/migration-tests/undecorated-derived-classes_expected.ts +++ /dev/null @@ -1,62 +0,0 @@ -import { Directive, NgModule, NgZone, Component } from '@angular/core'; -import { CheckboxControlValueAccessor, NG_VALUE_ACCESSOR, NG_ASYNC_VALIDATORS } from '@angular/forms'; -import { BaseComponentFromOtherFile, hostBindings } from './base-component'; - - -@Directive({ - selector: 'my-base-dir', - providers: [{provide: NgZone, useValue: null}] -}) -export class BaseDirective {} - -@Directive({ - selector: 'my-base-dir', - providers: [{ provide: NgZone, useValue: null }] -}) -export class DerivedDirective extends BaseDirective {} - -@Directive({ - selector: "input[type=checkbox][formControlName],input[type=checkbox][formControl],input[type=checkbox][ngModel]", - host: { - "(change)": "onChange($event.target.checked)", - "(blur)": "onTouched()" - }, - providers: [{ - provide: NG_VALUE_ACCESSOR, - useExisting: CheckboxControlValueAccessor, - multi: true - }] -}) -export class DerivedDirectiveFromNodeModules extends CheckboxControlValueAccessor {} - -@Component({ - selector: 'base-comp', - template: ` - This is the template. - `, - host: hostBindings, - providers: [ - { provide: NG_ASYNC_VALIDATORS, useValue: null }, - ], - // The following fields were copied from the base class, - // but could not be updated automatically to work in the - // new file location. Please add any required imports for - // the properties below: - styleUrls: nonExportedStyleUrlsVar -}) -export class DerivedComponentFromOtherSourceFile extends BaseComponentFromOtherFile {} - -@NgModule({ - declarations: [ - DerivedDirective, - DerivedDirectiveFromNodeModules, - DerivedComponentFromOtherSourceFile, - ], -}) -export class TestModule {} - - -@NgModule({ - declarations: [BaseDirective], -}) -export class BaseClassesModule {} diff --git a/integration/ng_update_migrations/test.js b/integration/ng_update_migrations/test.js index 3a19bb9ee4..75228ab1e5 100644 --- a/integration/ng_update_migrations/test.js +++ b/integration/ng_update_migrations/test.js @@ -18,8 +18,8 @@ const cliBinPath = path.join(projectDir, 'node_modules/@angular/cli/bin/ng'); const expectationFiles = glob.sync('**/*_expected.ts', {cwd: projectDir}); -const fromVersion = '8.0.0'; -const toVersion = '9.0.0'; +const fromVersion = '9.0.0'; +const toVersion = '10.0.0'; // Note that we need to specify "--allow-dirty" as the repository will become dirty // if dependencies for the integration test are installed (i.e. modified lock files) const updateCommandArgs = ['@angular/core', '--migrate-only', '--from', fromVersion, diff --git a/packages/core/schematics/migrations.json b/packages/core/schematics/migrations.json index 9d8c0e4803..e4a15c2d0c 100644 --- a/packages/core/schematics/migrations.json +++ b/packages/core/schematics/migrations.json @@ -20,30 +20,30 @@ "description": "Renderer to Renderer2 migration. As of Angular 9, the Renderer class is no longer available. Renderer2 should be used instead. Read more about this here: https://v9.angular.io/guide/migration-renderer", "factory": "./migrations/renderer-to-renderer2/index" }, - "migration-v9-missing-injectable": { - "version": "9.0.0-beta", - "description": "Missing @Injectable and incomplete provider definition migration. In Angular 9, enforcement of @Injectable decorators for DI is a bit stricter and incomplete provider definitions behave differently. Read more about this here: https://v9.angular.io/guide/migration-injectable", - "factory": "./migrations/missing-injectable/index" - }, "migration-v9-undecorated-classes-with-di": { "version": "9.0.0-beta", "description": "Undecorated classes with DI migration. As of Angular 9, it is no longer supported to use Angular DI on a class that does not have an Angular decorator. Read more about this here: https://v9.angular.io/guide/migration-undecorated-classes", "factory": "./migrations/undecorated-classes-with-di/index" }, - "migration-v9-undecorated-classes-with-decorated-fields": { - "version": "9.0.0-beta", - "description": "Undecorated classes with decorated fields migration. As of Angular 9, it is no longer supported to have Angular field decorators on a class that does not have an Angular decorator. Read more about this here: https://v9.angular.io/guide/migration-undecorated-classes", - "factory": "./migrations/undecorated-classes-with-decorated-fields/index" - }, "migration-v9-dynamic-queries": { "version": "9-beta", "description": "Static flag migration. Removes the `static` flag from dynamic queries. As of Angular 9, the \"static\" flag defaults to false and is no longer required for your view and content queries. Read more about this here: https://v9.angular.io/guide/migration-dynamic-flag", "factory": "./migrations/dynamic-queries/index" }, - "migration-v9-module-with-providers": { - "version": "9.0.0-beta", - "description": "ModuleWithProviders migration. In Angular 9, the ModuleWithProviders type without a generic has been deprecated. This migration adds the generic where it is missing. Read more about this here: https://v9.angular.io/guide/migration-module-with-providers", + "migration-v10-missing-injectable": { + "version": "10.0.0-beta", + "description": "Missing @Injectable and incomplete provider definition migration. As of Angular 9, enforcement of @Injectable decorators for DI is a bit stricter and incomplete provider definitions behave differently. Read more about this here: https://v9.angular.io/guide/migration-injectable", + "factory": "./migrations/missing-injectable/index" + }, + "migration-v10-module-with-providers": { + "version": "10.0.0-beta", + "description": "ModuleWithProviders migration. As of Angular 10, the ModuleWithProviders type requires a generic. This migration adds the generic where it is missing. Read more about this here: https://v10.angular.io/guide/migration-module-with-providers", "factory": "./migrations/module-with-providers/index" + }, + "migration-v10-undecorated-classes-with-decorated-fields": { + "version": "10.0.0-beta", + "description": "Undecorated classes with Angular features migration. In version 10, classes that use Angular features and do not have an Angular decorator are no longer supported. Read more about this here: https://v10.angular.io/guide/migration-undecorated-classes", + "factory": "./migrations/undecorated-classes-with-decorated-fields/index" } } } diff --git a/packages/core/schematics/test/missing_injectable_migration_spec.ts b/packages/core/schematics/test/missing_injectable_migration_spec.ts index 28e6787710..2fea3c5fe2 100644 --- a/packages/core/schematics/test/missing_injectable_migration_spec.ts +++ b/packages/core/schematics/test/missing_injectable_migration_spec.ts @@ -64,7 +64,7 @@ describe('Missing injectable migration', () => { } function runMigration() { - return runner.runSchematicAsync('migration-v9-missing-injectable', {}, tree).toPromise(); + return runner.runSchematicAsync('migration-v10-missing-injectable', {}, tree).toPromise(); } describe('NgModule', () => createTests('NgModule', 'providers')); diff --git a/packages/core/schematics/test/module_with_providers_migration_spec.ts b/packages/core/schematics/test/module_with_providers_migration_spec.ts index 13a999e76d..adf458e96c 100644 --- a/packages/core/schematics/test/module_with_providers_migration_spec.ts +++ b/packages/core/schematics/test/module_with_providers_migration_spec.ts @@ -230,6 +230,6 @@ describe('ModuleWithProviders migration', () => { } function runMigration() { - return runner.runSchematicAsync('migration-v9-module-with-providers', {}, tree).toPromise(); + return runner.runSchematicAsync('migration-v10-module-with-providers', {}, tree).toPromise(); } }); diff --git a/packages/core/schematics/test/undecorated_classes_with_decorated_fields_migration_spec.ts b/packages/core/schematics/test/undecorated_classes_with_decorated_fields_migration_spec.ts index 1866a7dfa8..2cf2815c2d 100644 --- a/packages/core/schematics/test/undecorated_classes_with_decorated_fields_migration_spec.ts +++ b/packages/core/schematics/test/undecorated_classes_with_decorated_fields_migration_spec.ts @@ -383,7 +383,7 @@ describe('Undecorated classes with decorated fields migration', () => { function runMigration() { return runner - .runSchematicAsync('migration-v9-undecorated-classes-with-decorated-fields', {}, tree) + .runSchematicAsync('migration-v10-undecorated-classes-with-decorated-fields', {}, tree) .toPromise(); } });