Skip to content

feat(migrations): add schematic to clean up unused imports#59353

Closed
crisbeto wants to merge 3 commits intoangular:mainfrom
crisbeto:unused-cleanup-script
Closed

feat(migrations): add schematic to clean up unused imports#59353
crisbeto wants to merge 3 commits intoangular:mainfrom
crisbeto:unused-cleanup-script

Conversation

@crisbeto
Copy link
Copy Markdown
Member

@crisbeto crisbeto commented Jan 3, 2025

In v19 we added a warning about unused standalone imports, however we didn't do anything about existing code which means that users have to clean it up manually. These changes add the ng g @angular/core:cleanup-unused-imports schematic which will remove the unused dependencies automatically.

There isn't any new detection code since all the manipulations are based on the produced diagnostics, but there's a bit of code to remove the import declarations from the file as well.

Fixes #58849.

Exports the error codes so that they can be reused.
Allows for user-defined options to be passed in when creating a program in tsurge.
In v19 we added a warning about unused standalone imports, however we didn't do anything about existing code which means that users have to clean it up manually. These changes add the `ng g @angular/core:cleanup-unused-imports` schematic which will remove the unused dependencies automatically.

There isn't any new detection code since all the manipulations are based on the produced diagnostics, but there's a bit of code to remove the import declarations from the file as well.

Fixes angular#58849.
@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer target: minor This PR is targeted for the next minor release labels Jan 3, 2025
@angular-robot angular-robot bot added detected: feature PR contains a feature commit area: migrations Issues related to `ng update`/`ng generate` migrations labels Jan 3, 2025
@ngbot ngbot bot added this to the Backlog milestone Jan 3, 2025
});
}

override async analyze(info: ProgramInfo): Promise<Serializable<CompilationUnitData>> {
Copy link
Copy Markdown
Member Author

@crisbeto crisbeto Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I'm not sure if I'm using the analyze method properly since it produces all the text replacements in addition to the analysis. The reason is that each file can be migrated independently and it doesn't require any information from other files (aside from typings). It was more hassle to encode the analysis results in a serializable way just to produce the same result in migrate than to produce the result immediately.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's totally fine to produce replacements in the analyze stage if they are easy to compute 👍. I think at this point you likely never generate replacements directly in migrate, but only ever filter replacements there (based on the global metadata).

@crisbeto crisbeto requested a review from devversion January 3, 2025 08:30
@crisbeto crisbeto marked this pull request as ready for review January 3, 2025 08:30
@pullapprove pullapprove bot removed the request for review from devversion January 3, 2025 08:30
@crisbeto crisbeto requested a review from devversion January 3, 2025 09:47
Copy link
Copy Markdown
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will review more as soon as possible!

});
}

override async analyze(info: ProgramInfo): Promise<Serializable<CompilationUnitData>> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's totally fine to produce replacements in the analyze stage if they are easy to compute 👍. I think at this point you likely never generate replacements directly in migrate, but only ever filter replacements there (based on the global metadata).

@JeanMeche
Copy link
Copy Markdown
Member

Q: Do we intend to add a doc entry for it ?

@crisbeto
Copy link
Copy Markdown
Member Author

crisbeto commented Jan 6, 2025

Yes, I'll send a follow-up with docs and potentially change the diagnostic message to mention the schematic.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 6, 2025
thePunderWoman pushed a commit that referenced this pull request Jan 6, 2025
…ge (#59353)

Allows for user-defined options to be passed in when creating a program in tsurge.

PR Close #59353
thePunderWoman pushed a commit that referenced this pull request Jan 6, 2025
In v19 we added a warning about unused standalone imports, however we didn't do anything about existing code which means that users have to clean it up manually. These changes add the `ng g @angular/core:cleanup-unused-imports` schematic which will remove the unused dependencies automatically.

There isn't any new detection code since all the manipulations are based on the produced diagnostics, but there's a bit of code to remove the import declarations from the file as well.

Fixes #58849.

PR Close #59353
@thePunderWoman
Copy link
Copy Markdown
Contributor

This PR was merged into the repository by commit d298d25.

The changes were merged into the following branches: main

@nook24
Copy link
Copy Markdown

nook24 commented Jan 9, 2025

Not sure if this is the right place to report this, but I tried the ng g @angular/core:cleanup-unused-imports command from the 19.1.0-rc.0 build.

Removed 3116 imports in 566 files

In my case, the command duplicated the TypeScript imports and added syntax errors like

import {
  CardBodyComponent,
  CardComponent,
  CardFooterComponent,
  CardHeaderComponent,
  CardTitleDirective,
  ColComponent,
  ContainerComponent,
  DropdownDividerDirective,
  FormControlDirective,
  FormDirective,
  InputGroupComponent,
  InputGroupTextDirective,
  ModalService,
  NavComponent,
  NavItemComponent,
  RowComponent,
  TableDirective
}{
  CardBodyComponent,
  CardComponent,
  CardFooterComponent,
  CardHeaderComponent,
  CardTitleDirective,
  ColComponent,
  ContainerComponent,
  DropdownDividerDirective,
  FormControlDirective,
  FormDirective,
  InputGroupComponent,
  InputGroupTextDirective,
  ModalService,
  NavComponent,
  NavItemComponent,
  RowComponent,
  TableDirective
} from '@coreui/angular';

The imports in the @Component decorator has the same issue:

@Component({
    selector: 'oitc-servicetemplategroups-index',
    imports: [
    TranslocoDirective,
    DeleteAllModalComponent,
    FaIconComponent,
    PermissionDirective,
    RouterLink,
    ActionsButtonComponent,
    ActionsButtonElementComponent,
    CardBodyComponent,
    CardComponent,
    CardFooterComponent,
    CardHeaderComponent,
    CardTitleDirective,
    ColComponent,
    ContainerComponent,
    DebounceDirective,
    DropdownDividerDirective,
    FormControlDirective,
    FormDirective,
    FormsModule,
    InputGroupComponent,
    InputGroupTextDirective,
    ItemSelectComponent,
    MatSort,
    MatSortHeader,
    NavComponent,
    NavItemComponent,
    NgForOf,
    NgIf,
    NoRecordsComponent,
    PaginateOrScrollComponent,
    ReactiveFormsModule,
    RowComponent,
    SelectAllComponent,
    TableDirective,
    TranslocoPipe,
    XsButtonDirective,
    TableLoaderComponent
][
    TranslocoDirective,
    DeleteAllModalComponent,
    FaIconComponent,
    PermissionDirective,
    RouterLink,
    ActionsButtonComponent,
    ActionsButtonElementComponent,
    CardBodyComponent,
    CardComponent,
    CardFooterComponent,
    CardHeaderComponent,
    CardTitleDirective,
    ColComponent,
    ContainerComponent,
    DebounceDirective,
    DropdownDividerDirective,
    FormControlDirective,
    FormDirective,
    FormsModule,
    InputGroupComponent,
    InputGroupTextDirective,
    ItemSelectComponent,
    MatSort,
    MatSortHeader,
    NavComponent,
    NavItemComponent,
    NgForOf,
    NgIf,
    NoRecordsComponent,
    PaginateOrScrollComponent,
    ReactiveFormsModule,
    RowComponent,
    SelectAllComponent,
    TableDirective,
    TranslocoPipe,
    XsButtonDirective,
    TableLoaderComponent
],
    templateUrl: './servicetemplategroups-index.component.html',
    styleUrl: './servicetemplategroups-index.component.css',
    providers: [
        { provide: DELETE_SERVICE_TOKEN, useClass: ServicetemplategroupsService } // Inject the ServicetemplategroupsService into the DeleteAllModalComponent
    ],
    changeDetection: ChangeDetectionStrategy.OnPush
})

@crisbeto
Copy link
Copy Markdown
Member Author

crisbeto commented Jan 9, 2025

@nook24 that does sound like a bug. Did this happen in all files or only specific ones?

@nook24
Copy link
Copy Markdown

nook24 commented Jan 9, 2025

This happened in a lot of files, so probably all that where touched by the command. In my case a few hundred files, I only checked a few of them they all had the same issue.

@crisbeto
Copy link
Copy Markdown
Member Author

crisbeto commented Jan 9, 2025

Interesting, I'm trying it against some of our own projects and it works fine. This looks like a string replacement issue, rather than the imports actually being duplicated. Do you happen to be on Windows? We've had issues in the past with text encoding there.

@nook24
Copy link
Copy Markdown

nook24 commented Jan 9, 2025

This was my first thought as well, that the string replace was gone wild. This is one of the files in question (original): https://gist.github.com/nook24/7d9141f4d82c071331f52d3fb1b43e6b

I run the command on Linux, with Linux new lines (\n).

Not sure if related, but the first run crashed with out of memory, increasing the system memory from 8 to 16GB resolved this.

# ng g @angular/core:cleanup-unused-imports                                                                                                                                    1 ✘ │ 08:38:49
    Preparing analysis for tsconfig.app.json
    Preparing analysis for tsconfig.spec.json
    Scanning for unused imports using tsconfig.app.json
    Scanning for unused imports using tsconfig.spec.json

<--- Last few GCs --->

[10167:0x301e5ef0]   173612 ms: Scavenge 2038.7 (2085.4) -> 2036.1 (2085.4) MB, 7.62 / 0.00 ms  (average mu = 0.236, current mu = 0.216) allocation failure;
[10167:0x301e5ef0]   173636 ms: Scavenge 2039.0 (2085.4) -> 2036.9 (2085.7) MB, 11.11 / 0.00 ms  (average mu = 0.236, current mu = 0.216) allocation failure;
[10167:0x301e5ef0]   173658 ms: Scavenge 2039.9 (2085.7) -> 2037.3 (2093.7) MB, 10.67 / 0.00 ms  (average mu = 0.236, current mu = 0.216) allocation failure;


<--- JS stacktrace --->

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
----- Native stack trace -----

 1: 0xb8d0a3 node::OOMErrorHandler(char const*, v8::OOMDetails const&) [ng g @angular/core:cleanup-unused-imports]
 2: 0xf06250 v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, v8::OOMDetails const&) [ng g @angular/core:cleanup-unused-imports]
 3: 0xf06537 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, v8::OOMDetails const&) [ng g @angular/core:cleanup-unused-imports]
 4: 0x11180d5  [ng g @angular/core:cleanup-unused-imports]
 5: 0x1118664 v8::internal::Heap::RecomputeLimits(v8::internal::GarbageCollector) [ng g @angular/core:cleanup-unused-imports]
 6: 0x112f554 v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::internal::GarbageCollectionReason, char const*) [ng g @angular/core:cleanup-unused-imports]
 7: 0x112fd6c v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [ng g @angular/core:cleanup-unused-imports]
 8: 0x1131eca v8::internal::Heap::HandleGCRequest() [ng g @angular/core:cleanup-unused-imports]
 9: 0x109d537 v8::internal::StackGuard::HandleInterrupts() [ng g @angular/core:cleanup-unused-imports]
10: 0x153fe32 v8::internal::Runtime_StackGuardWithGap(int, unsigned long*, v8::internal::Isolate*) [ng g @angular/core:cleanup-unused-imports]
11: 0x7f884fed9ef6
[1]    10167 IOT instruction (core dumped)  ng g @angular/core:cleanup-unused-imports

@crisbeto
Copy link
Copy Markdown
Member Author

crisbeto commented Jan 9, 2025

That file seems fine from what I can tell, although Github might be normalizing something. I can't run the file as is through our tests since all those imports need to actually exist for the migration to run.

@crisbeto
Copy link
Copy Markdown
Member Author

crisbeto commented Jan 9, 2025

I can get an identical result to yours if I comment out this line although I don't see a way that could happen in a migration. Can you try resetting your repo and running it from scratch with the extra memory? I wonder if the project wasn't left in a corrupted state during your first run when it ran out of memory.

@nook24
Copy link
Copy Markdown

nook24 commented Jan 9, 2025

In my case, the code is located at node_modules/@angular/core/schematics/bundles/cleanup-unused-imports.js, but it looks the same as the version you have posted in here.

Maybe i have installed the Angular version wrong, I used ng update @angular/core@19.1.0-rc.0 --force to get 19.1.0-rc.0.

Can you try resetting your repo and running it from scratch with the extra memory?

That was what I did in the first place. To be extra sure, I did it again, unfortunately with the same result. According to git, 283 files where changed in the process. I randomly picked 10 for manual inspection. All files I checked have the same issue. Also single line imports are affected (AsyncPipe, NgIf).

--- a/src/app/modules/import_module/components/dependency-tree/not-in-monitoring/not-in-monitoring.component.ts
+++ b/src/app/modules/import_module/components/dependency-tree/not-in-monitoring/not-in-monitoring.component.ts
@@ -1,8 +1,8 @@
 import { ChangeDetectionStrategy, Component, inject, Input } from '@angular/core';
-import { BadgeComponent, TableDirective } from '@coreui/angular';
+
 import { FaIconComponent } from '@fortawesome/angular-fontawesome';
-import { AsyncPipe, NgForOf, NgIf } from '@angular/common';
-import { PermissionDirective } from '../../../../../permissions/permission.directive';
+import { AsyncPipe, NgIf }{ AsyncPipe, NgIf } from '@angular/common';
+
 import { TranslocoDirective } from '@jsverse/transloco';
 import { NodeExtended } from '../dependency-tree.component';
 import { SystemnameService } from '../../../../../services/systemname.service';
@@ -10,15 +10,16 @@ import { SystemnameService } from '../../../../../services/systemname.service';
 @Component({
     selector: 'oitc-not-in-monitoring',
     imports: [
-        BadgeComponent,
-        FaIconComponent,
-        NgForOf,
-        NgIf,
-        PermissionDirective,
-        TableDirective,
-        TranslocoDirective,
-        AsyncPipe
-    ],
+    FaIconComponent,
+    NgIf,
+    TranslocoDirective,
+    AsyncPipe
+][
+    FaIconComponent,
+    NgIf,
+    TranslocoDirective,
+    AsyncPipe
+],
     templateUrl: './not-in-monitoring.component.html',
     styleUrl: './not-in-monitoring.component.css',
     changeDetection: ChangeDetectionStrategy.OnPush

nook24 added a commit to openITCOCKPIT/openITCOCKPIT-frontend-angular that referenced this pull request Jan 9, 2025
@crisbeto
Copy link
Copy Markdown
Member Author

crisbeto commented Jan 9, 2025

Do those problematic files happen to have BOM? In vscode you can see it in the bottom right:
Screenshot 2025-01-09 at 11 22 55

@nook24
Copy link
Copy Markdown

nook24 commented Jan 9, 2025

Normal UTF-8 files before and after the migration:
grafik

Could this be caused by the fact that the Angular 19 update removed all standalone: true lines? This also confuses the latest version of the VS Code language server:
grafik

@crisbeto
Copy link
Copy Markdown
Member Author

crisbeto commented Jan 9, 2025

Hmm then I'm a bit out of ideas without being able to reproduce it locally. One last thing we can try is to replace the insertLeft in node_modules/@angular/core/schematics/bundles/cleanup-unused-imports.js to insertRight.

@nook24
Copy link
Copy Markdown

nook24 commented Jan 9, 2025

insertRight resolved the issue for me. I attached the new diff, where everything got replaced as expected.

--- a/src/app/modules/import_module/components/dependency-tree/not-in-monitoring/not-in-monitoring.component.ts
+++ b/src/app/modules/import_module/components/dependency-tree/not-in-monitoring/not-in-monitoring.component.ts
@@ -1,8 +1,8 @@
 import { ChangeDetectionStrategy, Component, inject, Input } from '@angular/core';
-import { BadgeComponent, TableDirective } from '@coreui/angular';
+
 import { FaIconComponent } from '@fortawesome/angular-fontawesome';
-import { AsyncPipe, NgForOf, NgIf } from '@angular/common';
-import { PermissionDirective } from '../../../../../permissions/permission.directive';
+import { AsyncPipe, NgIf } from '@angular/common';
+
 import { TranslocoDirective } from '@jsverse/transloco';
 import { NodeExtended } from '../dependency-tree.component';
 import { SystemnameService } from '../../../../../services/systemname.service';
@@ -10,15 +10,11 @@ import { SystemnameService } from '../../../../../services/systemname.service';
 @Component({
     selector: 'oitc-not-in-monitoring',
     imports: [
-        BadgeComponent,
-        FaIconComponent,
-        NgForOf,
-        NgIf,
-        PermissionDirective,
-        TableDirective,
-        TranslocoDirective,
-        AsyncPipe
-    ],
+    FaIconComponent,
+    NgIf,
+    TranslocoDirective,
+    AsyncPipe
+],
     templateUrl: './not-in-monitoring.component.html',
     styleUrl: './not-in-monitoring.component.css',
     changeDetection: ChangeDetectionStrategy.OnPush

Updated 283 files in this case.

Many thanks for your time and effort ❤️

crisbeto added a commit to crisbeto/angular that referenced this pull request Jan 9, 2025
Based on angular#59353 (comment), fixes a text replacement issue that seems to cause code to be duplicated in some setups.
@crisbeto
Copy link
Copy Markdown
Member Author

crisbeto commented Jan 9, 2025

Awesome, thank you for verifying it! 🎉 I've sent a fix at #59452.

kirjs pushed a commit that referenced this pull request Jan 10, 2025
Based on #59353 (comment), fixes a text replacement issue that seems to cause code to be duplicated in some setups.

PR Close #59452
kirjs pushed a commit that referenced this pull request Jan 10, 2025
Based on #59353 (comment), fixes a text replacement issue that seems to cause code to be duplicated in some setups.

PR Close #59452
PrajaktaB27 pushed a commit to PrajaktaB27/angular that referenced this pull request Feb 7, 2025
Exports the error codes so that they can be reused.

PR Close angular#59353
PrajaktaB27 pushed a commit to PrajaktaB27/angular that referenced this pull request Feb 7, 2025
…ge (angular#59353)

Allows for user-defined options to be passed in when creating a program in tsurge.

PR Close angular#59353
PrajaktaB27 pushed a commit to PrajaktaB27/angular that referenced this pull request Feb 7, 2025
…9353)

In v19 we added a warning about unused standalone imports, however we didn't do anything about existing code which means that users have to clean it up manually. These changes add the `ng g @angular/core:cleanup-unused-imports` schematic which will remove the unused dependencies automatically.

There isn't any new detection code since all the manipulations are based on the produced diagnostics, but there's a bit of code to remove the import declarations from the file as well.

Fixes angular#58849.

PR Close angular#59353
PrajaktaB27 pushed a commit to PrajaktaB27/angular that referenced this pull request Feb 7, 2025
Based on angular#59353 (comment), fixes a text replacement issue that seems to cause code to be duplicated in some setups.

PR Close angular#59452
@angular-automatic-lock-bot
Copy link
Copy Markdown

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-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: migrations Issues related to `ng update`/`ng generate` migrations detected: feature PR contains a feature commit target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Auto remove unused standalone imports

5 participants