feat(core): add ng generate schematic to remove unnecessary modules #48832

crisbeto · 2023-01-25T08:54:41Z

Adds a new mode to the @angular/core:standalone schematic that automatically deletes modules that may not be necessary after the previous step of converting them to standalone.

Adds a new mode to the `@angular/core:standalone` schematic that automatically deletes modules that may not be necessary after the previous step of converting them to standalone.

alan-agius4 · 2023-01-25T12:27:46Z

packages/core/schematics/ng-generate/standalone-migration/schema.json

@@ -15,6 +15,10 @@
          {


Out of curiosity why these migration are being added in ng generate and not ng update?

Because we don't want apps to be switched to standalone automatically when they update. Also this migration isn't 100% foolproof so we want authors to verify between each step of the migration.

In such cases typically optional migrations were used before. This was the case also for similar migrations in the framework such as typed form migrations which required users to run these manually.

ng update @angular/core --name=typed-forms --migrate-only

There will be more steps to this schematic. The first step converts declarations to standalone, then users need to verify that their app works, then they can run this one to remove any modules that aren't necessary anymore. After that there's another step to switch from bootstrapModule to bootstrapApplication. These steps are also set up so they can be run against specific directories so the entire app doesn't have to be switched over at once.

packages/core/schematics/ng-generate/standalone-migration/schema.json

devversion

Overall LGTM, but a small amount of comments/questions

devversion · 2023-01-25T13:03:41Z

packages/core/schematics/ng-generate/standalone-migration/prune-modules.ts

+  // We can't remove classes where the `imports` contain something different
+  // from an identifier, because it may be a `ModuleWithProviders`.
+  if (imports && isNonEmptyNgModuleProperty(imports) &&
+      imports.initializer.elements.some(el => !ts.isIdentifier(el))) {


What about module with providers that are exposed through identifiers? I think this is possible, kind of like:

// routes.ts export const routes = RouterModule.forRoot({..}) // some-module.ts @NgModule({ imports: [routes], })

In a lot of places you could use the partial evaluator to make this more robust, but I do acknowledge that we do not necessarily try to support all cases. Mostly bringing this up to see what you think about it. Usage of partial evaluation is rather simple anyway (you have all the parts already)

Yeah, I was thinking of using the evaluator, but I wanted to keep the initial implementation simple and see how many cases we miss when testing against real apps.

Okay, a pattern like above seems quite likely (personal experience seeing couple of projects), but doing it in a follow-up then, sounds good. We can keep this conversation unresolved to keep it visible

packages/core/schematics/ng-generate/standalone-migration/prune-modules.ts

devversion · 2023-01-25T13:12:18Z

packages/core/schematics/test/standalone_migration_spec.ts

+      import {NgModule} from '@angular/core';
+
+      @NgModule({})
+      export class ModuleA {}


What should happen if this module defines providers? AppModule would not have any imports anymore, loose the providers and there would not be a todo at all right?

That's a good point, in this case we'll incorrectly remove the module. I'll address this together with a few other bugs I found while testing against the Material docs app.

crisbeto · 2023-01-25T13:29:33Z

I've addressed some of the feedback and I'll circle back to the module removal issue when fixing some of the other bugs I found during testing.

…dules

devversion

LGTM, we can address the two comments in follow-ups

jessicajaniuk · 2023-01-25T16:20:29Z

This PR was merged into the repository by commit e7318fc.

…ransitively Fixes that we would incorrectly remove a module that imports another module which has providers. This is a follow-up from the following discussion: angular#48832 (comment)

…ransitively (#48866) Fixes that we would incorrectly remove a module that imports another module which has providers. This is a follow-up from the following discussion: #48832 (comment) PR Close #48866

robertIsaac · 2023-01-27T20:32:11Z

@crisbeto can we add extended diagnostic to report unnecessary modules in the future?

crisbeto · 2023-01-27T20:38:22Z

Extended diagnostics are meant for component templates, but it should be straightforward to turn this into a lint rule.

robertIsaac · 2023-01-28T13:08:41Z

Extended diagnostics are meant for component templates, but it should be straightforward to turn this into a lint rule.

So it should be added to angular eslint?

…ngular#48832) Adds a new mode to the `@angular/core:standalone` schematic that automatically deletes modules that may not be necessary after the previous step of converting them to standalone. PR Close angular#48832

…ransitively (angular#48866) Fixes that we would incorrectly remove a module that imports another module which has providers. This is a follow-up from the following discussion: angular#48832 (comment) PR Close angular#48866

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@angular/animations](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fanimations/15.1.5/15.2.0) | | [@angular/common](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fcommon/15.1.5/15.2.0) | | [@angular/compiler](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/15.1.5/15.2.0) | | [@angular/compiler-cli](https://github.com/angular/angular/tree/main/packages/compiler-cli) ([source](https://github.com/angular/angular)) | devDependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/15.1.5/15.2.0) | | [@angular/core](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fcore/15.1.5/15.2.0) | | [@angular/forms](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fforms/15.1.5/15.2.0) | | [@angular/platform-browser](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/15.1.5/15.2.0) | | [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/15.1.5/15.2.0) | --- ### Release Notes <details> <summary>angular/angular</summary> ### [`v15.2.0`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#1520-2023-02-22) [Compare Source](angular/angular@15.1.5...15.2.0) #### Deprecations ##### - Class and `InjectionToken` guards and resolvers are deprecated. Instead, write guards as plain JavaScript functions and inject dependencies with `inject` from `@angular/core`. ##### | Commit | Type | Description | | -- | -- | -- | | [926c35f4ac](angular/angular@926c35f) | docs | Deprecate class and InjectionToken and resolvers ([#47924](angular/angular#47924)) | ##### common | Commit | Type | Description | | -- | -- | -- | | [54b24eb40f](angular/angular@54b24eb) | feat | Add loaderParams attribute to NgOptimizedImage ([#48907](angular/angular#48907)) | ##### compiler-cli | Commit | Type | Description | | -- | -- | -- | | [0cf11167f1](angular/angular@0cf1116) | fix | incorrectly detecting forward refs when symbol already exists in file ([#48988](angular/angular#48988)) | ##### core | Commit | Type | Description | | -- | -- | -- | | [a154db8a81](angular/angular@a154db8) | feat | add ng generate schematic to convert declarations to standalone ([#48790](angular/angular#48790)) | | [345e737daa](angular/angular@345e737) | feat | add ng generate schematic to convert to standalone bootstrapping APIs ([#48848](angular/angular#48848)) | | [e7318fc758](angular/angular@e7318fc) | feat | add ng generate schematic to remove unnecessary modules ([#48832](angular/angular#48832)) | ##### language-service | Commit | Type | Description | | -- | -- | -- | | [4ae384fd61](angular/angular@4ae384f) | feat | Allow auto-imports of a pipe via quick fix when its selector is used, both directly and via reexports. ([#48354](angular/angular#48354)) | | [141333411e](angular/angular@1413334) | feat | Introduce a new NgModuleIndex, and use it to suggest re-exports. ([#48354](angular/angular#48354)) | | [d0145033bd](angular/angular@d014503) | fix | generate forwardRef for same file imports ([#48898](angular/angular#48898)) | ##### migrations | Commit | Type | Description | | -- | -- | -- | | [2796230e95](angular/angular@2796230) | fix | add `enum` in `mode` option in `standalone` schema ([#48851](angular/angular#48851)) | | [816e76a578](angular/angular@816e76a) | fix | automatically prune root module after bootstrap step ([#49030](angular/angular#49030)) | | [bdbf21d04b](angular/angular@bdbf21d) | fix | avoid generating imports with forward slashes ([#48993](angular/angular#48993)) | | [32cf4e5cb9](angular/angular@32cf4e5) | fix | avoid internal modules when generating imports ([#48958](angular/angular#48958)) | | [521ccfbe6c](angular/angular@521ccfb) | fix | avoid interrupting the migration if language service lookup fails ([#49010](angular/angular#49010)) | | [a40cd47aa7](angular/angular@a40cd47) | fix | avoid modifying testing modules without declarations ([#48921](angular/angular#48921)) | | [1afa6ed322](angular/angular@1afa6ed) | fix | don't add ModuleWithProviders to standalone test components ([#48987](angular/angular#48987)) | | [c98c6a8452](angular/angular@c98c6a8) | fix | don't copy animations modules into the imports of test components ([#49147](angular/angular#49147)) | | [8389557848](angular/angular@8389557) | fix | don't copy unmigrated declarations into imports array ([#48882](angular/angular#48882)) | | [f82bdc4b01](angular/angular@f82bdc4) | fix | don't delete classes that may provide dependencies transitively ([#48866](angular/angular#48866)) | | [759db12e0b](angular/angular@759db12) | fix | duplicated comments on migrated classes ([#48966](angular/angular#48966)) | | [ba38178d19](angular/angular@ba38178) | fix | generate forwardRef for same file imports ([#48898](angular/angular#48898)) | | [03fcb36cfd](angular/angular@03fcb36) | fix | migrate HttpClientModule to provideHttpClient() ([#48949](angular/angular#48949)) | | [2de6dae16d](angular/angular@2de6dae) | fix | migrate RouterModule.forRoot with a config object to use features ([#48935](angular/angular#48935)) | | [770191cf1f](angular/angular@770191c) | fix | migrate tests when switching to standalone bootstrap API ([#48987](angular/angular#48987)) | | [c7926b5773](angular/angular@c7926b5) | fix | move standalone migrations into imports ([#48987](angular/angular#48987)) | | [65c74ed93e](angular/angular@65c74ed) | fix | normalize paths to posix ([#48850](angular/angular#48850)) | | [6377487b1a](angular/angular@6377487) | fix | only exclude bootstrapped declarations from initial standalone migration ([#48987](angular/angular#48987)) | | [e9e4449a43](angular/angular@e9e4449) | fix | preserve tsconfig in standalone migration ([#48987](angular/angular#48987)) | | [ffad1b49d9](angular/angular@ffad1b4) | fix | reduce number of files that need to be checked ([#48987](angular/angular#48987)) | | [ba7a757cc5](angular/angular@ba7a757) | fix | return correct alias when conflicting import exists ([#49139](angular/angular#49139)) | | [49a7c9f94a](angular/angular@49a7c9f) | fix | standalone migration incorrectly throwing path error for multi app projects ([#48958](angular/angular#48958)) | | [584976e6c8](angular/angular@584976e) | fix | support --defaults in standalone migration ([#48921](angular/angular#48921)) | | [03f47ac901](angular/angular@03f47ac) | fix | use consistent quotes in generated imports ([#48876](angular/angular#48876)) | | [ebae506d89](angular/angular@ebae506) | fix | use import remapper in root component ([#49046](angular/angular#49046)) | | [40c976c909](angular/angular@40c976c) | fix | use NgForOf instead of NgFor ([#49022](angular/angular#49022)) | | [4ac25b2aff](angular/angular@4ac25b2) | perf | avoid re-traversing nodes when resolving bootstrap call dependencies ([#49010](angular/angular#49010)) | | [26cb7ab2e6](angular/angular@26cb7ab) | perf | speed up language service lookups ([#49010](angular/angular#49010)) | ##### platform-browser | Commit | Type | Description | | -- | -- | -- | | [bf4ad38117](angular/angular@bf4ad38) | fix | remove styles from DOM of destroyed components ([#48298](angular/angular#48298)) | ##### platform-server | Commit | Type | Description | | -- | -- | -- | | [25e220a23a](angular/angular@25e220a) | fix | avoid duplicate TransferState info after renderApplication call ([#49094](angular/angular#49094)) | ##### router | Commit | Type | Description | | -- | -- | -- | | [31b94c762f](angular/angular@31b94c7) | feat | Add a withNavigationErrorHandler feature to provideRouter ([#48551](angular/angular#48551)) | | [dedac8d3f7](angular/angular@dedac8d) | feat | Add test helper for trigger navigations in tests ([#48552](angular/angular#48552)) | #### Special Thanks Alan Agius, Alex Castle, Alex Rickabaugh, Andrew Kushnir, Andrew Scott, Dylan Hunn, Ikko Eltociear Ashimine, Ilyass, Jessica Janiuk, Joey Perrott, John Manners, Kalbarczyk, Kristiyan Kostadinov, Matthieu Riegler, Paul Gschwendtner, Pawel Kozlowski, Virginia Dooley, Walid Bouguima, cexbrayat and mgechev  </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1793 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

angular-automatic-lock-bot · 2023-02-28T16:06:19Z

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

crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime area: migrations Issues related to `ng update` migrations target: minor This PR is targeted for the next minor release labels Jan 25, 2023

ngbot bot modified the milestone: Backlog Jan 25, 2023

angular-robot bot added the detected: feature PR contains a feature commit label Jan 25, 2023

crisbeto force-pushed the standalone-migration-module-pruning branch from 0bf5f05 to b5f0e84 Compare January 25, 2023 08:56

feat(core): add ng generate schematic to remove unnecessary modules

504aca5

Adds a new mode to the `@angular/core:standalone` schematic that automatically deletes modules that may not be necessary after the previous step of converting them to standalone.

crisbeto force-pushed the standalone-migration-module-pruning branch from b5f0e84 to 504aca5 Compare January 25, 2023 09:08

crisbeto requested a review from devversion January 25, 2023 09:21

crisbeto marked this pull request as ready for review January 25, 2023 09:21

alan-agius4 reviewed Jan 25, 2023

View reviewed changes

packages/core/schematics/ng-generate/standalone-migration/schema.json Show resolved Hide resolved

devversion reviewed Jan 25, 2023

View reviewed changes

fixup! feat(core): add ng generate schematic to remove unnecessary mo…

ceef40c

…dules

crisbeto force-pushed the standalone-migration-module-pruning branch from a770b77 to ceef40c Compare January 25, 2023 14:01

devversion approved these changes Jan 25, 2023

View reviewed changes

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 25, 2023

jessicajaniuk closed this in e7318fc Jan 25, 2023

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): add ng generate schematic to remove unnecessary modules #48832

feat(core): add ng generate schematic to remove unnecessary modules #48832

crisbeto commented Jan 25, 2023

alan-agius4 Jan 25, 2023

crisbeto Jan 25, 2023 •

edited

alan-agius4 Jan 25, 2023 •

edited

crisbeto Jan 25, 2023

devversion left a comment

devversion Jan 25, 2023

crisbeto Jan 25, 2023

devversion Jan 25, 2023

devversion Jan 25, 2023

crisbeto Jan 25, 2023

crisbeto commented Jan 25, 2023

devversion left a comment

jessicajaniuk commented Jan 25, 2023

robertIsaac commented Jan 27, 2023

crisbeto commented Jan 27, 2023

robertIsaac commented Jan 28, 2023

angular-automatic-lock-bot bot commented Feb 28, 2023

feat(core): add ng generate schematic to remove unnecessary modules #48832

feat(core): add ng generate schematic to remove unnecessary modules #48832

Conversation

crisbeto commented Jan 25, 2023

alan-agius4 Jan 25, 2023

Choose a reason for hiding this comment

crisbeto Jan 25, 2023 • edited

Choose a reason for hiding this comment

alan-agius4 Jan 25, 2023 • edited

Choose a reason for hiding this comment

crisbeto Jan 25, 2023

Choose a reason for hiding this comment

devversion left a comment

Choose a reason for hiding this comment

devversion Jan 25, 2023

Choose a reason for hiding this comment

crisbeto Jan 25, 2023

Choose a reason for hiding this comment

devversion Jan 25, 2023

Choose a reason for hiding this comment

devversion Jan 25, 2023

Choose a reason for hiding this comment

crisbeto Jan 25, 2023

Choose a reason for hiding this comment

crisbeto commented Jan 25, 2023

devversion left a comment

Choose a reason for hiding this comment

jessicajaniuk commented Jan 25, 2023

robertIsaac commented Jan 27, 2023

crisbeto commented Jan 27, 2023

robertIsaac commented Jan 28, 2023

angular-automatic-lock-bot bot commented Feb 28, 2023

crisbeto Jan 25, 2023 •

edited

alan-agius4 Jan 25, 2023 •

edited