@angular/core update schematic v9 to v10 adds odd todo comments in a correct-looking file #37726

kroeder · 2020-06-25T06:21:16Z

🐞 bug report

Affected Package

The issue is caused by package @angular/core

Description

ng update @angular/core from v9 to v10 adds multiple comments that tell me I need to change something which I did not expect

> 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
    Could not migrate all undecorated classes that use Angular features.
    Please manually fix the following failures:
    ⮑   projects\client-core\src\lib\i18n\model-i18n\translateField.pipe.ts@6:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.

The code looks like this

import { AccessPathLike } from '@aeb/graphql-accessor';
import { ChangeDetectorRef, OnDestroy, Pipe, PipeTransform } from '@angular/core';
import { Subscription } from 'rxjs';
import { ModelLocalizationService } from './model-localization.service';

// TODO: Add Angular decorator.
// TODO: Add Angular decorator.
// TODO: Add Angular decorator.
// TODO: Add Angular decorator.
// TODO: Add Angular decorator.
// TODO: Add Angular decorator.
@Pipe({
    name: 'translateField',
    pure: false
})
export class TranslateFieldPipe implements PipeTransform, OnDestroy {
    private subscription: Subscription | undefined = undefined;
    // don't initialize with undefined because then the pipe might break consumers expecting string
    private currentValue: string = '';
    private currentPath: AccessPathLike | undefined = undefined;

    constructor(
        private readonly modelLocalizationService: ModelLocalizationService,
        private readonly changeDetectorRef: ChangeDetectorRef
    ) {
    }

    ngOnDestroy() {
        if (this.subscription) {
            this.subscription.unsubscribe();
            this.subscription = undefined;
        }
    }

    transform(value: AccessPathLike|undefined): string {
        if (value === this.currentPath) {
            return this.currentValue;
        }
        this.currentPath = value;
        if (this.subscription) {
            this.subscription.unsubscribe();
            this.subscription = undefined;
        }
        if (!value) {
            return '';
        }
        this.subscription = this.modelLocalizationService.watchFieldLabel(value).subscribe(label => this.updateValue(label));
        return this.currentValue;
    }

    private updateValue(value: string) {
        this.currentValue = value;
        this.changeDetectorRef.markForCheck();
    }
}

It already has a decorator. I assume this is a bug and I can't do anything to satisfy ng update @angular/core ?

🔬 Minimal Reproduction

I'm not sure if the above example alone is enough for a reproduction on your side. Let me know if you need more!

🌍 Your Environment

     _                      _                 ____ _     ___
    / \   _ __   __ _ _   _| | __ _ _ __     / ___| |   |_ _|
   / △ \ | '_ \ / _` | | | | |/ _` | '__|   | |   | |    | |
  / ___ \| | | | (_| | |_| | | (_| | |      | |___| |___ | |
 /_/   \_\_| |_|\__, |\__,_|_|\__,_|_|       \____|_____|___|
                |___/
    

Angular CLI: 10.0.0
Node: 10.19.0
OS: win32 x64

Angular: 10.0.0
... animations, cdk, cli, common, compiler, compiler-cli, core
... forms, language-service, platform-browser
... platform-browser-dynamic, router
Ivy Workspace: No

Package                            Version
------------------------------------------------------------
@angular-devkit/architect          0.900.7
@angular-devkit/build-angular      0.1000.0
@angular-devkit/build-ng-packagr   0.1000.0
@angular-devkit/build-optimizer    0.1000.0
@angular-devkit/build-webpack      0.1000.0
@angular-devkit/core               10.0.0
@angular-devkit/schematics         10.0.0
@angular/flex-layout               10.0.0-beta.32
@ngtools/webpack                   10.0.0
@schematics/angular                10.0.0
@schematics/update                 0.1000.0
ng-packagr                         10.0.0
rxjs                               6.5.4
typescript                         3.9.5
webpack                            4.43.0

The text was updated successfully, but these errors were encountered:

kroeder · 2020-06-25T06:36:59Z

More details

I noticed it added these todos to 2 pure: false pipes (out of 3 - so might not be the reason)
It also added these todos to a single service (out of plenty of services in my repo)

// TODO: Add Angular decorator.
// TODO: Add Angular decorator.
// TODO: Add Angular decorator.
// TODO: Add Angular decorator.
// TODO: Add Angular decorator.
// TODO: Add Angular decorator.
@Pipe({
    name: 'nxTimeAgo',
    pure: false
})
export class NxTimeAgoPipe implements PipeTransform, OnDestroy {

and

// TODO: Add Angular decorator.
// TODO: Add Angular decorator.
// TODO: Add Angular decorator.
// TODO: Add Angular decorator.
// TODO: Add Angular decorator.
// TODO: Add Angular decorator.
@Injectable({
    providedIn: 'root'
})

export class NavigationService implements OnDestroy

This one is not affected

@Pipe({
    name: 'nxNumber',
    pure: false
})
export class NumberPipe implements PipeTransform {

All other pure pipes don't have todo comments

I first thought it is related to implements OnDestroy. All my examples implement it but I found one service that is not affected and has OnDestroy implemented.

A previous schematic also throws a warning that is related to this issue

> 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
    Could not migrate all instances of ModuleWithProviders
    Please manually fix the following failures:
    ⮑   projects\client-ui\src\lib\pipes\time-ago\time-ago.module.ts@26:5: Method type is not statically analyzable.
    ⮑   projects\client-ui\src\lib\pipes\time-ago\time-ago.module.ts@26:5: Method type is not statically analyzable.
    ⮑   projects\client-ui\src\lib\pipes\time-ago\time-ago.module.ts@26:5: Method type is not statically analyzable.
    ⮑   projects\client-ui\src\lib\pipes\time-ago\time-ago.module.ts@26:5: Method type is not statically analyzable.
    ⮑   projects\client-ui\src\lib\pipes\time-ago\time-ago.module.ts@26:5: Method type is not statically analyzable.
    ⮑   projects\client-ui\src\lib\pipes\time-ago\time-ago.module.ts@26:5: Method type is not statically analyzable.
  Migration completed.

time-ago.module.ts

import { CommonModule } from '@angular/common';
import { NgModule } from '@angular/core';
import { TimeagoClock, TimeagoCustomFormatter, TimeagoDefaultClock, TimeagoFormatter, TimeagoIntl, TimeagoModule, TimeagoModuleConfig } from 'ngx-timeago';
import { CustomTimeAgoIntl } from './custom-time-ago-intl';
import { NxTimeAgoPipe } from './time-ago.pipe';

@NgModule({
    imports: [
        CommonModule,
        TimeagoModule
    ],
    declarations: [
        NxTimeAgoPipe
    ],
    exports: [
        NxTimeAgoPipe
    ]
})
export class TimeAgoPipeModule {
    static forRoot(config: TimeagoModuleConfig = {}) {
        return {
            ngModule: TimeAgoPipeModule,
            imports: [
                TimeagoModule.forRoot()
            ],
            providers: [
                config.intl || { provide: TimeagoIntl, useClass: CustomTimeAgoIntl },
                config.clock || { provide: TimeagoClock, useClass: TimeagoDefaultClock },
                config.formatter || { provide: TimeagoFormatter, useClass: TimeagoCustomFormatter }
            ]
        };
    }
}

Full error message

> 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
    Could not migrate all undecorated classes that use Angular features.
    Please manually fix the following failures:
    ⮑   projects\client-core\src\lib\i18n\model-i18n\translateField.pipe.ts@6:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.
    ⮑   projects\client-ui\src\lib\pipes\time-ago\time-ago.pipe.ts@5:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.
    ⮑   projects\client-ui\src\lib\components\menus\navigation\navigation.service.ts@8:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.
    ⮑   projects\client-ui\src\lib\pipes\time-ago\time-ago.pipe.ts@6:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.
    ⮑   projects\client-ui\src\lib\components\menus\navigation\navigation.service.ts@9:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.
    ⮑   projects\client-core\src\lib\i18n\model-i18n\translateField.pipe.ts@7:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.
    ⮑   projects\client-core\src\lib\i18n\model-i18n\translateField.pipe.ts@8:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.
    ⮑   projects\client-core\src\lib\i18n\model-i18n\translateField.pipe.ts@9:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.
    ⮑   projects\client-ui\src\lib\pipes\time-ago\time-ago.pipe.ts@7:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.
    ⮑   projects\client-ui\src\lib\components\menus\navigation\navigation.service.ts@10:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.
    ⮑   projects\client-core\src\lib\i18n\model-i18n\translateField.pipe.ts@10:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.
    ⮑   projects\client-ui\src\lib\pipes\time-ago\time-ago.pipe.ts@8:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.
    ⮑   projects\client-ui\src\lib\components\menus\navigation\navigation.service.ts@11:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.
    ⮑   projects\client-core\src\lib\i18n\model-i18n\translateField.pipe.ts@11:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.
    ⮑   projects\client-ui\src\lib\pipes\time-ago\time-ago.pipe.ts@9:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.
    ⮑   projects\client-ui\src\lib\components\menus\navigation\navigation.service.ts@12:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.
    ⮑   projects\client-ui\src\lib\pipes\time-ago\time-ago.pipe.ts@10:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.
    ⮑   projects\client-ui\src\lib\components\menus\navigation\navigation.service.ts@13:1: Class uses Angular features but cannot be migrated automatically. Please add an appropriate Angular decorator.

leon · 2020-06-25T07:53:20Z

Also noted that angular adds the same error to services that implement OnDestroy

// TODO: Add Angular decorator.
@Injectable()
export class MyService implements OnDestroy {
}

HMubaireek · 2020-06-25T09:00:05Z

I noticed the same.

It added so many TODO comments to a guard I have:

@Injectable({
  providedIn: 'root'
})
export class LoginGuard implements CanActivate, OnDestroy {

And for a service I have, it added @Directive():

@Directive()
@Injectable({
  providedIn: 'root'
})
export class SignalrHelperService implements OnDestroy {

As of v10, the `undecorated-classes-with-decorated-fields` migration generally deals with undecorated classes using Angular features. We intended to run this migation as part of v10 again as undecorated classes with Angular features are no longer supported in planned v11. The migration currently behaves incorrectly in some cases where an `@Injectable` or `@Pipe` decorated classes uses the `ngOnDestroy` lifecycle hook. We incorrectly add a TODO for those classes. This commit fixes that. Also while being at it, this commit fixes an issue where multiple TODO's could be added. This happens when multiple Angular CLI build targets have an overlap in source files. Multiple programs then capture the same source file, causing the migration to detect an undecorated class multiple times (i.e. adding a TODO twice). Fixes angular#37726.

As of v10, the `undecorated-classes-with-decorated-fields` migration generally deals with undecorated classes using Angular features. We intended to run this migation as part of v10 again as undecorated classes with Angular features are no longer supported in planned v11. The migration currently behaves incorrectly in some cases where an `@Injectable` or `@Pipe` decorated classes uses the `ngOnDestroy` lifecycle hook. We incorrectly add a TODO for those classes. This commit fixes that. Additionally, this change makes the migration more robust to not migrate a class if it inherits from a component, pipe injectable or non-abstract directive. We previously did not need this as the undecorated-classes-with-di migration ran before, but this is no longer the case. Last, this commit fixes an issue where multiple TODO's could be added. This happens when multiple Angular CLI build targets have an overlap in source files. Multiple programs then capture the same source file, causing the migration to detect an undecorated class multiple times (i.e. adding a TODO twice). Fixes angular#37726.

snebjorn · 2020-06-25T13:53:43Z

I had something similar but completely different ;)

@Directive() // it added this
export abstract class AbstractComponent implements OnInit, OnDestroy {

It's a Component not a Directive
It's abstract and thous I'd say that it's not necessary
Removing @Directive makes the compiler complain but the app works

devversion · 2020-06-25T14:33:12Z

@snebjorn It seems that this is the expected behavior. Your class might be suffixed with Component, but as long as it does not have any component decorator, it's not a component. Since you use Angular features within the class, adding @Decorator is needed so that the Angular compiler can generate a definition.

As of version 9, it's required to explicitly decorate such classes. Just using abstract class is not sufficient. The Angular compiler will only process the class if it has @Directive applied. more details in: https://angular.io/guide/migration-undecorated-classes

Removing @directive makes the compiler complain but the app works

It will break though when you use @HostBinding, queries, DI or bind to inputs etc. Is there any issue with using @Directive here?

snebjorn · 2020-06-25T16:25:05Z

Is there any issue with using @Directive here?

None critical. But it's confusing when looking through the code as it looks like a derived Component is inheriting from a Directive. So unless you're aware of this requirement then it looks like an error.

I also had to disable my linting rule
// tslint:disable-next-line: directive-class-suffix

As of v10, the `undecorated-classes-with-decorated-fields` migration generally deals with undecorated classes using Angular features. We intended to run this migation as part of v10 again as undecorated classes with Angular features are no longer supported in planned v11. The migration currently behaves incorrectly in some cases where an `@Injectable` or `@Pipe` decorated classes uses the `ngOnDestroy` lifecycle hook. We incorrectly add a TODO for those classes. This commit fixes that. Additionally, this change makes the migration more robust to not migrate a class if it inherits from a component, pipe injectable or non-abstract directive. We previously did not need this as the undecorated-classes-with-di migration ran before, but this is no longer the case. Last, this commit fixes an issue where multiple TODO's could be added. This happens when multiple Angular CLI build targets have an overlap in source files. Multiple programs then capture the same source file, causing the migration to detect an undecorated class multiple times (i.e. adding a TODO twice). Fixes angular#37726.

…#37732) As of v10, the `undecorated-classes-with-decorated-fields` migration generally deals with undecorated classes using Angular features. We intended to run this migation as part of v10 again as undecorated classes with Angular features are no longer supported in planned v11. The migration currently behaves incorrectly in some cases where an `@Injectable` or `@Pipe` decorated classes uses the `ngOnDestroy` lifecycle hook. We incorrectly add a TODO for those classes. This commit fixes that. Additionally, this change makes the migration more robust to not migrate a class if it inherits from a component, pipe injectable or non-abstract directive. We previously did not need this as the undecorated-classes-with-di migration ran before, but this is no longer the case. Last, this commit fixes an issue where multiple TODO's could be added. This happens when multiple Angular CLI build targets have an overlap in source files. Multiple programs then capture the same source file, causing the migration to detect an undecorated class multiple times (i.e. adding a TODO twice). Fixes #37726. PR Close #37732

…angular#37732) As of v10, the `undecorated-classes-with-decorated-fields` migration generally deals with undecorated classes using Angular features. We intended to run this migation as part of v10 again as undecorated classes with Angular features are no longer supported in planned v11. The migration currently behaves incorrectly in some cases where an `@Injectable` or `@Pipe` decorated classes uses the `ngOnDestroy` lifecycle hook. We incorrectly add a TODO for those classes. This commit fixes that. Additionally, this change makes the migration more robust to not migrate a class if it inherits from a component, pipe injectable or non-abstract directive. We previously did not need this as the undecorated-classes-with-di migration ran before, but this is no longer the case. Last, this commit fixes an issue where multiple TODO's could be added. This happens when multiple Angular CLI build targets have an overlap in source files. Multiple programs then capture the same source file, causing the migration to detect an undecorated class multiple times (i.e. adding a TODO twice). Fixes angular#37726. PR Close angular#37732

- ran update schematics to v10 and fixed tests - due to new TS version introduced with Angular v10 upgrade, e2e tests needed modifications to compile properly, and a previous transitive dependency needed to be explicitly added - Angular CLI added comments to higlight manual actions needed after migration, but no action was needed (see angular/angular#37726) - correctly updated Angular dependencies in lib package, moving tslib from peerDependencies to dependencies as suggested by build warning

angular-automatic-lock-bot · 2020-07-26T16:14:51Z

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

…angular#37732) As of v10, the `undecorated-classes-with-decorated-fields` migration generally deals with undecorated classes using Angular features. We intended to run this migation as part of v10 again as undecorated classes with Angular features are no longer supported in planned v11. The migration currently behaves incorrectly in some cases where an `@Injectable` or `@Pipe` decorated classes uses the `ngOnDestroy` lifecycle hook. We incorrectly add a TODO for those classes. This commit fixes that. Additionally, this change makes the migration more robust to not migrate a class if it inherits from a component, pipe injectable or non-abstract directive. We previously did not need this as the undecorated-classes-with-di migration ran before, but this is no longer the case. Last, this commit fixes an issue where multiple TODO's could be added. This happens when multiple Angular CLI build targets have an overlap in source files. Multiple programs then capture the same source file, causing the migration to detect an undecorated class multiple times (i.e. adding a TODO twice). Fixes angular#37726. PR Close angular#37732

JoostK added the comp: migrations label Jun 25, 2020

ngbot bot added this to the needsTriage milestone Jun 25, 2020

devversion self-assigned this Jun 25, 2020

devversion mentioned this issue Jun 25, 2020

fix(migrations): do not incorrectly add todo for @Injectable or @Pipe #37732

Closed

devversion added freq2: medium severity2: inconvenient state: confirmed labels Jun 25, 2020

AndrewKushnir closed this in angular/angular@d12cdb5 Jun 25, 2020

pkozlowski-opensource mentioned this issue Jun 29, 2020

build: updated Angular to v10 ng-bootstrap/ng-bootstrap#3790

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 26, 2020

angular / angular Public

@angular/core update schematic v9 to v10 adds odd todo comments in a correct-looking file #37726

@angular/core update schematic v9 to v10 adds odd todo comments in a correct-looking file #37726

kroeder commented Jun 25, 2020 •

edited

kroeder commented Jun 25, 2020 •

edited

leon commented Jun 25, 2020

HMubaireek commented Jun 25, 2020 •

edited

snebjorn commented Jun 25, 2020

devversion commented Jun 25, 2020

snebjorn commented Jun 25, 2020

angular-automatic-lock-bot bot commented Jul 26, 2020

angular / angular Public

@angular/core update schematic v9 to v10 adds odd todo comments in a correct-looking file #37726

@angular/core update schematic v9 to v10 adds odd todo comments in a correct-looking file #37726

Comments

kroeder commented Jun 25, 2020 • edited

🐞 bug report

Affected Package

Description

🔬 Minimal Reproduction

🌍 Your Environment

kroeder commented Jun 25, 2020 • edited

leon commented Jun 25, 2020

HMubaireek commented Jun 25, 2020 • edited

snebjorn commented Jun 25, 2020

devversion commented Jun 25, 2020

snebjorn commented Jun 25, 2020

angular-automatic-lock-bot bot commented Jul 26, 2020

kroeder commented Jun 25, 2020 •

edited

kroeder commented Jun 25, 2020 •

edited

HMubaireek commented Jun 25, 2020 •

edited